Couple of comments:

 - I think the patch description actually has to be "resolve issue1611" 
not "resolve issue 1611" - though perhaps we should improve the 
recogniser. This is in screened now so something to remember for the 
future, rather than changing this patch.

 - The message shouldn't be printed if the user did actually supply --
edit-long-comment

 - The comment "refactoring needed to get witnesses compiling" refers to 
a change you made rather than the new state of the code, and therefore 
better belongs in the patch comment rather than the code itself. I'm 
ambivalent about whether the comment is needed at all, but if we keep it 
something like "auxiliary function needed because the witness types 
differ for the isTag case" would be better.

 - How about a regression test?

>From my point of view tags should be filtered out in case you are just
trying to add new changes to a patch... maybe warning with "Tag X was
skipped" in a similar way that you warn when trying to amend a dependent
patch. Anyway, I fixed it in a easy way and it is better than nothing.
NB: I'm assuming that it should be legal any kind of tag metadata update.

On Sun, Apr 3, 2011 at 12:01 PM, Ganesh Sittampalam <bugs@darcs.net> wrote:

>
> New submission from Ganesh Sittampalam <ganesh@earth.li>:
>
> Couple of comments:
>
>  - I think the patch description actually has to be "resolve issue1611"
> not "resolve issue 1611" - though perhaps we should improve the
> recogniser. This is in screened now so something to remember for the
> future, rather than changing this patch.
>
As I said you I write it in that way because googling I saw some examples
that follow that pattern
http://www.google.pt/search?q=%22resolve+issue%22+darcs , today I re-visited
those examples and I see that most of them are from Eric, Florent, etc (i.e.
core team members). Was the convention different at some point in the past?

>
>  - The message shouldn't be printed if the user did actually supply --
> edit-long-comment
>
>  - The comment "refactoring needed to get witnesses compiling" refers to
> a change you made rather than the new state of the code, and therefore
> better belongs in the patch comment rather than the code itself. I'm
> ambivalent about whether the comment is needed at all, but if we keep it
> something like "auxiliary function needed because the witness types
> differ for the isTag case" would be better.
>
>  - How about a regression test?


> ----------
> status: needs-screening -> review-in-progress
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch575>
> __________________________________
>

-- 
Iago Abal Rivas

Attachments

• improve-resolution-of-issue1611.dpatch
• unnamed

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-9b6da6188663e096ac130714ebb47dddabe82dbe

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-9b6da6188663e096ac130714ebb47dddabe82dbe

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-9b6da6188663e096ac130714ebb47dddabe82dbe