[darcs-devel] [patch2038] finally complete transition to option system

Ben Franksen bugs at darcs.net
Sun Jul 19 23:13:36 UTC 2020


Ben Franksen <ben.franksen at online.de> added the comment:

>>   * avoid use of matchAny in commands
> 
> OK (I'll assume the change is correct)

It is. The change means we interpret only those MatchFlags that the
command specifies (and documents!).

>> +-- use of RemoteRepo data constructor is harmless here, if not ideal
> 
> This is a bit unclear without looking at the comment below about
> accessing the flag list, and could get out of date if that gets changed.
> But not a big deal.

Good point though, see patch2060.

>>   * remove option type parameter from DarcsCommand
>>   
>>   This is basically a complete rollback of
>>   
>>   patch 6ef3e49796ef64ae23bde58bfc4ea6720e8fd14d
>>   
>>   Made possible since we no longer do any low-level DarcsFlag hacking.
> 
> I barely remember the history of this, although the new state looks fine
> to me. From what I recall/understand from this patch:
> 
> - We started transitioning from [DarcsFlag] to strongly typed option
> records for each command, but didn't get very far and it's now on hold
> or abandoned, but not rolled back. An example of a migrated command is
> 'amend'.

Yes. My take is that the final verdict here is still open, but I tend to
think that the effort should be abandoned. I found that this approach is
just too inflexible. Removing an option or merely changing the order of
options (which affects only the help output) entails a lot of churn, and
the benefits are negligable in practice. I have indeed thought about
rolling back this part, i.e. the config records in the amend and record
commands, but couldn't get myself motivated enough to actually do it.
Perhaps things would be different if we had anonymous records and at
some point in the future we might actually get them, or a good enough
approximation, which is another reason I haven't done a rollback yet.

> - I added this type parameter/explicit config translation step to make
> it hard to forget to normalise [DarcsFlag] for commands that still use
> it directly.
> 
> - Now we still have lots of commands that use [DarcsFlag] for options,
> but because they treat it opaquely, the explicit normalise step isn't
> needed.
> 
> Is that right?

Yes, that sums it up pretty well.

> I also find onormalise/defaultFlags in D.UI.Options.Core a bit confusing
> now, although it's not directly touched by this patch. onormalise is now
> only used by defaultFlags. Both of them have specifications that are
> identical to their bodies. Perhaps there's some scope for simplification
> here.

It would be possible to inline onormalize now, but I don't think there
is much scope for simplification beyond that, because oparse and
ounparse are primitives. We'd end up with

  defaultFlags opts = (oparse opts . ounparse opts) id opts []

which I find harder to grok than the two separate functions. I am
usually keen to inline functions that are used only in one place, but in
this case I think keeping them separate makes it easier to understand
what goes on.

However, the docs for onormalize are outdated, i.e. the half-sentence

"which is useful as long as there is legacy code that circumvents the
'OptSpec' abstraction and directly tests for flag membership"

can and should be removed, see patch2061.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2038>
__________________________________


More information about the darcs-devel mailing list