[darcs-devel] [patch1179] Darcs.Repository.Flags: added Eq instanc... (and 4 more)

[Ben Franksen]

Hi Ganesh

>> Ganesh Sittampalam wrote:
> 
>>> The negatives are that it's a bit more complicated than the old system
>>> and the type signatures for the sets of options in each command are
>>> pretty verbose.
>> 
>> Yes, they are horrible, and it is part of why I had some reservations
>> about the whole idea. They make me feel like programming in one of these
>> first order statically typed languages (C, Java, Pascal). A proper record
>> system for Haskell (with row polymorphism) would make this a lot nicer...
> 
> I'm playing around with moving them inline to the DarcsCommand
> specifications and avoiding the boilerplate odesc/defaultFlags/ocheck.
> If it works out, it'll mean they can be just specified once there and
> avoid needing to give a signature anywhere.

I'd be astonished if you get this to work, because of the type parameters, 
which are (necessarily, that's a feature) different for each command. The 
Option 'member functions' odesc, oparse, etc kind of 'equalize' these 
differences: they have simple monomorphic result types like [DarcsFlag]. I 
have been playing with existentially quantifying over the parameters but 
that really just moves the ugliness from one place to another.

>> I remember that getting *all* tests to succeed again was a major headache
>> -- the last few took me a few days and some restructuring. The problem is
>> that the new way to define options requires you to think about when and
>> how options interact with each other. I think this is a good thing, but
>> it can make it hard to achieve full backwards compatibility.
>> 
>> I'll take a look at the convert.sh problem and see if I find a nice
>> solution.
> 
> FYI I now know what's wrong here - the problem is that the list of flags
> including the defaults isn't being squashed, so we end up with something
> like [UseFormat2, UseFormat1] being passed to commands.
> 
> Code which does things like UseFormat1 `elem` flags assuming they are
> mutually exclusive goes wrong. I'm actually amazed that only one test
> ran into this.

and in a follow-up you write:
> Just to add, the obvious fix for this specific problem is to change the
> (UseFormat1 `elem` flags) code to be (parseFlags O.patchFormat flags ==
> PatchFormat1).

There were lots of tests that failed at first because of exactly this 
problem. I usually isolated them to some function in D.UI.Flags and then re-
wrote that function to avoid the membership tests in favor of using the new 
Options system in the manner you indicated.

I have just sent a patch that fixes the convert.sh test along these lines. 
Note that this also shows how to get rid of flag insertions, such as is done 
in the toDarcs2 function: instead, we add a silent option (with the correct 
default).

> This would be fixed in the long-run by converting all commands to the
> new options record style.

Yes.

> In the shorter term we can hopefully force the
> flags lists to be normalised by passing through oparse and ounparse.

This is possible, but (see above) you need to do it separately in every 
command implementation (AFAICS).

I did not do this because I think it is a bad idea, since it covers up the 
problem instead of fixing it. I really want to get rid of all the explicit 
flag membership testing. If a test fails this is an incentive to do just 
that. I think the patch I sent demonstrates that doing this is mostly 
straight forward.

and later:
> The worry is how many other places similar things may be happening; I'm
> not too comfortable passing around unnormalised [DarcsFlag] values as
> who knows how they might be used.

I know it will be some work to review all the places where the flag list is 
used directly, but I think we'll have to do it anyway, the sooner the 
better.

Now that the new options system is in screened, we should also make it clear 
that using [DarcsFlag] in any way other than as an opaque Config value is 
strongly discouraged, especially in new code.

Cheers
Ben
-- 
"Make it so they have to reboot after every typo." -- Scott Adams