make tests work, implement 'set noparent', owners propagating down

make tests work, implement 'set noparent', owners propagating down

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77351

[Dirk Pranke, 2011-03-07 20:50:27]

There's a fair amount of change in this file, as all the logic needed to change
to implement "set noparent" and OWNERS files propagating down by default.
Fortunately, the code is still small, so hopefully it is all reviewable in a
single block. Let me know if you'd like me to split this up into smaller chunks.

[Dirk Pranke, 2011-03-07 21:03:39]

Okay, landed the other changed and rebased ... this should make things easier to
follow.

[M-A Ruel, 2011-03-08 20:24:02]

A lots of nits but nothing show stopping,

lgtm

http://codereview.chromium.org/6627059/diff/5001/owners.py
File owners.py (right):

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode28
owners.py:28: BASIC_EMAIL_REGEXP = r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$'
style nit:
I prefer const at the top of the file, and 2 empty lines between file-level
symbols.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode50
owners.py:50: self.email_regexp = re.compile(BASIC_EMAIL_REGEXP)
FYI, if you want it to be a whitelist of valid credentials, it probably needs to
be either a functor or a list of valid regexp or something.

I think it's out of scope of this change and I'm fine with this design for now
and just fix it once it's needed.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode90
owners.py:90: def _CheckPaths(self, files):
BTW I'm trying to get towards PEP-8 style naming for new code but I don't mind
either.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode93
owners.py:93: assert all(_isunder(f, self.root) for f in files)
What if self.root is not an abspath already? Like a symlink

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode95
owners.py:95: def _CheckReviewers(self, reviewers):
Can you add a one line docstring saying that it "Verifies that each reviewer is
a valid email address". While I'm not a fan of docstrings, it think it's useful
here.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode135
owners.py:135: self._ReadOneLine(line, lineno, dirpath, owners_path)
I don't think having this code in a function makes it more readable.

http://codereview.chromium.org/6627059/diff/5001/tests/filesystem_mock.py
File tests/filesystem_mock.py (right):

http://codereview.chromium.org/6627059/diff/5001/tests/filesystem_mock.py#new...
tests/filesystem_mock.py:41: if self.sep not in path:
I always forgot the subtle difference there. (?)

http://codereview.chromium.org/6627059/diff/5001/tests/owners_unittest.py
File tests/owners_unittest.py (right):

http://codereview.chromium.org/6627059/diff/5001/tests/owners_unittest.py#new...
tests/owners_unittest.py:124: self.assert_SyntaxError('set
myparentisbillgates\n')
mydadisbillgates would be more specific. :)

[Dirk Pranke, 2011-03-08 22:05:26]

http://codereview.chromium.org/6627059/diff/5001/owners.py
File owners.py (right):

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode28
owners.py:28: BASIC_EMAIL_REGEXP = r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$'
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> style nit:
> I prefer const at the top of the file, and 2 empty lines between file-level
> symbols.

OK.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode50
owners.py:50: self.email_regexp = re.compile(BASIC_EMAIL_REGEXP)
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> FYI, if you want it to be a whitelist of valid credentials, it probably needs
to
> be either a functor or a list of valid regexp or something.
> 
> I think it's out of scope of this change and I'm fine with this design for now
> and just fix it once it's needed.

Yeah, this was intended to be the simplest possible thing that would work for
now, and when I get to the TODO it'll probably need to be a validation callback
or something.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode90
owners.py:90: def _CheckPaths(self, files):
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> BTW I'm trying to get towards PEP-8 style naming for new code but I don't mind
> either.

You tell me this NOW?? :) Is there anything else in your style that isn't PEP-8
besides the two-space indent?

I'll change the naming in a later patch. I'd be happy to do anything that pushes
us to converging styles between Chromium and WebKit.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode93
owners.py:93: assert all(_isunder(f, self.root) for f in files)
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> What if self.root is not an abspath already? Like a symlink

Good question. I'll update it.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode95
owners.py:95: def _CheckReviewers(self, reviewers):
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> Can you add a one line docstring saying that it "Verifies that each reviewer
is
> a valid email address". While I'm not a fan of docstrings, it think it's
useful
> here.

Will do.

http://codereview.chromium.org/6627059/diff/5001/owners.py#newcode135
owners.py:135: self._ReadOneLine(line, lineno, dirpath, owners_path)
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> I don't think having this code in a function makes it more readable.

I slightly prefer it, I think, but I agree it's not much of a win.

http://codereview.chromium.org/6627059/diff/5001/tests/filesystem_mock.py
File tests/filesystem_mock.py (right):

http://codereview.chromium.org/6627059/diff/5001/tests/filesystem_mock.py#new...
tests/filesystem_mock.py:41: if self.sep not in path:
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> I always forgot the subtle difference there. (?)

I think it's mostly a question of operator precedence; there was real change in
this particular diff, I just prefer the latter style.

http://codereview.chromium.org/6627059/diff/5001/tests/owners_unittest.py
File tests/owners_unittest.py (right):

http://codereview.chromium.org/6627059/diff/5001/tests/owners_unittest.py#new...
tests/owners_unittest.py:124: self.assert_SyntaxError('set
myparentisbillgates\n')
On 2011/03/08 20:24:03, Marc-Antoine Ruel wrote:
> mydadisbillgates would be more specific. :)

Heh. Done.