Code review comment for lp:~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern

Revision history for this message
Robert Collins (lifeless) wrote :

So, this is comprehensive, and I like that. I have a general point,
and a couple of specifics. I haven't done a line by line reading [yet,
 I will after the general point is discussed].

Firstly, the general point. I think its fine, if the ignore file is
damaged to just stop. add doesn't need to be able to succeed when the
environment is broken. Stop (let the exception propogate) and have the
user fix the issue. Better that than adding something they wanted
ignored, I think. Similarly for status.

Now, I might be misreading, and thats what you're doing : but if thats
what you're doing, its leaking up into code that it shouldn't. So I'd
like to see this patch with no changes to code that uses ignores - no
changes should be needed to do what I've described. This is because I
don't think the approach of scattering try/excepts around in higher
level code makes sense: push it down into the ignores module.

Secondly, the specific issue - to expose the real re compile function,
just assign it to a variable that can be accessed: wrappers of the
form:
def a_foo(param):
   result = real_foo(param)
   return result

can be rewritten as
def a_foo(param):
    return real_foo(param)

which is precisely equivalent to
a_foo = real_foo

So in this case, just call the _real_re_compile function, as is. No
changes needed.

sidenote: I rather suspect theres a bunch of edits here we may end up
not wanting, and perhaps we'll want to flatten the history to avoid
that overhead.

-Rob

« Back to merge proposal