[PATCH] bookmarks: simplify code

[Kevin Bullock]

On Oct 17, 2012, at 2:15 PM, Adrian Buehlmann wrote:

> On 2012-10-17 19:21, Kevin Bullock wrote:
>> # HG changeset patch
>> # User Kevin Bullock <kbullock at ringworld.org>
>> # Date 1350494123 18000
>> # Node ID 4dc03350b9e8e0e6e25e3796465d0d65463d16da
>> # Parent  9469bbd2f3a1e39c086383f3fe1543b801991b6f
>> bookmarks: simplify code
>> 
>> Remove some unnecessary return statements and collect some checks into
>> one place. As requested by Thomas Arendsen Hein <thomas at intevation.de>.
>> 
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -813,19 +813,18 @@ def bookmark(ui, repo, mark=None, rev=No
>>         raise util.Abort(_("--rev is incompatible with --delete"))
>>     if rename and rev:
>>         raise util.Abort(_("--rev is incompatible with --rename"))
>> +    if mark is None and (delete or rev):
>> +        raise util.Abort(_("bookmark name required"))
>> 
>>     if delete:
>> -        if mark is None:
>> -            raise util.Abort(_("bookmark name required"))
>>         if mark not in marks:
>>             raise util.Abort(_("bookmark '%s' does not exist") % mark)
>>         if mark == repo._bookmarkcurrent:
>>             bookmarks.setcurrent(repo, None)
>>         del marks[mark]
>>         bookmarks.write(repo)
>> -        return
>> -
>> -    if rename:
>> +
>> +    elif rename:
>>         if mark is None:
>>             raise util.Abort(_("new bookmark name required"))
>>         mark = checkformat(mark)
>> @@ -837,9 +836,8 @@ def bookmark(ui, repo, mark=None, rev=No
>>             bookmarks.setcurrent(repo, mark)
>>         del marks[rename]
>>         bookmarks.write(repo)
>> -        return
>> -
>> -    if mark is not None:
>> +
>> +    elif mark is not None:
>>         mark = checkformat(mark)
>>         if inactive and mark == repo._bookmarkcurrent:
>>             bookmarks.setcurrent(repo, None)
>> @@ -854,17 +852,14 @@ def bookmark(ui, repo, mark=None, rev=No
>>         bookmarks.write(repo)
>>         return
>> 
>> -    if mark is None:
>> -        if rev:
>> -            raise util.Abort(_("bookmark name required"))
>> +    else: # mark is None
> 
> IMHO, this is an example of a bad comment. Don't describe in different
> words what the code is already doing. The comment may be a lie (perhaps
> not today, but in the future).

If it were anywhere except on a bare 'else' that's a goodly distance removed from the other corresponding clauses, I'd agree. If the comment becomes a lie, then the code within the 'else' clause also becomes invalid. That being said, the 'else' isn't *so* far from the prior 'elif' that I'd be upset if the comment disappeared.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20121017/fe8f2618/attachment.html>