[PATCH 2 of 5 STABLE] largefiles: exit from remove with 1 on warnings
matt_harbison at yahoo.com
Sat Sep 8 20:36:14 CDT 2012
Greg Ward wrote:
> On 06 September 2012, Matt Harbison said:
>>>> def warn(files, reason):
>>>> + ret = 0
>>>> for f in files:
>>>> ui.warn(_('not removing %s: %s (use forget to undo)\n')
>>>> % (m.rel(f), reason))
>>>> + ret = 1
>>>> + return ret
> [to which I replied]
>>> Hmmm. What about:
>>> --- a/hgext/largefiles/overrides.py
>>> +++ b/hgext/largefiles/overrides.py
>>> @@ -141,6 +141,7 @@
>>> for f in files:
>>> ui.warn(_('not removing %s: %s (use forget to undo)\n')
>>> % (m.rel(f), reason))
>>> + return int(len(f)> 0)
>>> if after:
>>> remove, forget = deleted, 
> [spurring this response]
>> Seems good to me. I'm not a python programmer by trade
> I am, and it looks fine to me. Well, except I screwed up the
> whitespace -- *sigh*.
>> Maybe it would be better though to take this
>> out of the closure entirely? I wasn't thrilled with 'result =
>> warn(..) or result', but with this, it can probably be a one liner
>> after all of the warnings are emitted.
> No, I think you've got the right pattern. First, warn() is called in
> several places; second, it might be called with an empty list. You
> only want the non-zero return value when the list is non-empty, and
> warn() is the obvious place to make that call.
> As for "result = warn(...) or result": it's slightly odd, but a
> similar pattern emerges when writing Mercurial hooks too. It's a
> slight conflation of booleans and integers, but what the hell, if it's
> good enough for Kernighan and Ritchie, it's good enough for me. ;-)
Heh, I abuse int/bool all the time, so I didn't even think about that.
What I meant was some people prefer to avoid conditionals or logical
operands with side effects- in this case printing the warning. (I was
originally going to put 'result = result or warn(..)' so the 'or result'
part doesn't fade into the parameter list of warn(), but then the
warning wouldn't always be printed).
FWIW, I was saying something like this:
remove, forget = deleted, 
warn(modified + added + clean, _('file still exists'))
+ result = int(len(clean)>0)
remove, forget = deleted + clean, 
warn(modified, _('file is modified'))
warn(added, _('file has been marked for add'))
+ result = result or int(len(modified)>0 or len(added)>0)
Which I guess isn't as readable as I thought it would be. So as long as
there are no rules against logical operands with side effects, I'll make
the adjustment to warn().
More information about the Mercurial-devel