Patch 1609 make type Repository abstract

Title make type Repository abstract
Superseder Nosy List bf
Related Issues
Status accepted Assigned To

Created on 2017-10-04.05:05:30 by bf, last changed 2018-02-07.22:05:05 by gh.

File name Status Uploaded Type Edit Remove
make-type-repository-abstract.dpatch bf, 2017-10-04.05:05:29 application/x-darcs-patch
renamed-pristine-to-pristinetype-and-repohaspristine-to-repopristinetype.dpatch bf, 2017-10-27.11:20:36 text/x-darcs-patch
See mailing list archives for discussion on individual patches.
msg19693 (view) Author: bf Date: 2017-10-04.05:05:29
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Thu Sep 28 20:17:26 CEST 2017
  * make type Repository abstract
  We no longer export the data constructor and instead add a number of
  accessor functions and one convenience wrapper (withRepoLocation). Renamed
  the existing accessor function extractCache to repoCache for consistency.
  Also added witness coercion functions and a single function to construct a
  new Repository that is used only in D.R.Identify and D.R.Clone.
  Despite the large number of changes in this patch, it is completely
  harmless. It consists mostly of mechanical fixes everywhere we
  pattern-matched on the Repo data constructor before.
msg19694 (view) Author: bf Date: 2017-10-04.05:52:45
I should, perhaps, remark on why I did this.

One reason is that this way of using pattern matching for the arguments
of top-level functions is abusive, IMO. It expresses a preference to
give a name to /things/, whereas the major strength of functional
programming is to give names to (small, re-usable) /functions/.

And, of course, there are the usual reasons: we can now add or remove
data members from Repository without having to change all functions that
take it as argument.

Last not least: we used pattern matching for two completely separate
reasons: (1) to get at (some of) the members and (2) to reconstruct an
identical copy with a different type, i.e. with "new" witnesses. These
two motives are now expressed with separate functions, making the
intention clear.
msg19708 (view) Author: gh Date: 2017-10-06.12:40:08
I agree that things are much cleaner with this patch, and your arguments
supporting the changes.

I like the new withRepoLocation function.

The introduction of coerceT/coerceU and such are useful, it avoids
artificially building another Repo from an existing one just to make the
types new.

One minor thing I would nitpick is the name repoHasPristine in place of
repoPristine (in the code, the situation of not having a pristine is
represented by a NoPristine pristine).

msg19709 (view) Author: bf Date: 2017-10-06.13:12:22
I do not like the name repoHasPristine either, but repoPristine sounded
too much like it returns the pristine tree for me, when in fact it just
returns an enumeration describing what kind of pristine we have.

Perhaps rename data Pristine to data PristineType, then rename
repoHasPristine to repoPristineType?
msg19788 (view) Author: bf Date: 2017-10-27.11:20:36
Attached is a bundle with a single patch that renames Pristine to
PristineType and repoHasPristine to repoPristineType.
msg19841 (view) Author: gh Date: 2018-02-07.22:05:05
Accepting the followup patch too.
Date User Action Args
2017-10-04 05:05:30bfcreate
2017-10-04 05:52:46bfsetstatus: needs-screening -> needs-review
messages: + msg19694
2017-10-06 12:40:09ghsetstatus: needs-review -> accepted
messages: + msg19708
2017-10-06 13:12:22bfsetmessages: + msg19709
2017-10-27 11:20:37bfsetfiles: + renamed-pristine-to-pristinetype-and-repohaspristine-to-repopristinetype.dpatch
messages: + msg19788
2018-02-07 22:05:05ghsetmessages: + msg19841