Code review comment for lp:~parthm/bzr/300062-invalid-pattern-warnings

110 + if validp:
111 + g = Globster(validp)
112 + new_regex_patterns.append(g.regex_patterns[0])

^- This seems odd. A comment here saying that we are collapsing the valid requests into a single compiled regex would be useful.

158 +def is_pattern_valid(pattern):

^- It seems like it would be nice if this could share the code that determines what translation unit to use. Otherwise it seems likely to get skew if one of them is updated. For example, the proposed 'FixedString' prefix. The fallout is that it would normally work fine, but if someone had a *different* invalid regex, then suddenly it would start causing this one to fail as well.

I don't understand why you changed variables from being private. In lazy_regex you renamed _real_re_compile to real_re_compile, and in globbing you changed _regex_patterns.

It is perfectly fine for *tests* to check private variables, it is good to keep them private so that plugins get a feeling that they may not be stable names.

...

342 + def test_invalid_pattern_in_ignore_file(self):
343 + """Existing invalid patterns should be shown.
344 + """
345 + tree = self.make_branch_and_tree('.')
346 + self.build_tree(['file', 'foo.c'])
347 + f = open('.bzrignore', 'w')
348 + try:
349 + f.write(u"*.c\nRE:[\n") # invalid pattern
350 + finally:
351 + f.close()

^- I think you can simplify this to:
self.build_tree_contents([('.bzrignore', '*.c\nRE:[\n')])

It is pretty bad form to write *unicode* strings to f.write() given that in python2 it is an 8-bit request. (It happens to be automatically casting your unicode string down to an ascii string.)

This test might be easier to read if we turned it into a "run_script" test, but that may not be easily available here.

The same comments apply to test_add_with_invalid_ignore_in_global_ignore and
test_invalid_pattern_in_ignore_file in the 2 other places.