[darcs-devel] [patch536] Maybefy arguments to rollback (and 5 more)

gh bugs at darcs.net
Tue Feb 1 16:13:16 UTC 2011


gh <guillaumh at gmail.com> added the comment:

Thanks for the explanation Alexey. Also looking at the previous bundle
review at http://bugs.darcs.net/patch178 was helpful to me.

The weakness in my review is that I don't know the code enough to tell
where the conversion to this new semantics has to be done, but since
you mention you're going to write a followup bundle, I'm trusting you
on that.


Also Alexey, could you could move the functions announceFiles and
filterExistingFiles outside of Darcs.Command.Whatsnew in your next
patch bundle? It does not look great to have modules like
Darcs.Command.Revert and Darcs.Command.AmendRecord that import
Darcs.Command.Whatnew just for that.

review below:

2011/1/27 Alexey Levan <bugs at darcs.net>:
>
> New submission from Alexey Levan <exlevan at gmail.com>:
>
> 6 patches for repository http://darcs.net/screened:
>
> Wed Jan 26 18:52:29 EET 2011  Alexey Levan <exlevan at gmail.com>
>  * Maybefy arguments to rollback
>
> Wed Jan 26 21:27:59 EET 2011  Alexey Levan <exlevan at gmail.com>
>  * Maybefy arguments to record
>
> Wed Jan 26 22:56:24 EET 2011  Alexey Levan <exlevan at gmail.com>
>  * Maybefy arguments to whatsnew

The 3 patches above look correct and are very similar one another:
they "convert" an empty list of paths into Nothing, which means "apply
this command to no specific file".

> Wed Jan 26 23:22:27 EET 2011  Alexey Levan <exlevan at gmail.com>
>  * Split Darcs.Commands.WhatsNew.announceFiles

Before the above patch, announceFiles was doing 2 things: printing a
list of files on the terminal and returning the list of correct paths
from the given list. Now the behaviour is separated in two functions,
it's better.

>
> Thu Jan 27 01:22:25 EET 2011  Alexey Levan <exlevan at gmail.com>
>  * Don't treat empty lists as special; use Maybe instead

OK essentially the above pathc is about making the functions
unrecordedChanges and readUnrecorded (from Darcs.Repository.State) use
that new "maybe semantics". So the patch name is not very clear, but
well.

>
> Thu Jan 27 01:26:54 EET 2011  Alexey Levan <exlevan at gmail.com>
>  * Remove unused areFileArgs

OK.


I have pushed the bundle to screened but not to darcs.net yet due to
dependencies on other patches not yet in darcs.net .

Guillaume

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


More information about the darcs-devel mailing list