[webkit-reviews] review granted: [Bug 33454] check-webkit-style: Create a class to represent a list of filter rules. : [Attachment 46261] Proposed patch 3

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has granted Chris Jerdonek
<chris.jerdonek at gmail.com>'s request for review:
Bug 33454: check-webkit-style: Create a class to represent a list of filter
rules.
https://bugs.webkit.org/show_bug.cgi?id=33454

Attachment 46261: Proposed patch 3
https://bugs.webkit.org/attachment.cgi?id=46261&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
All concerns are resolved. Thanks!

> > Why don't we strip the arguments?
> ... snipped ...
> Does that seem reasonable to you?

I see. Thanks for the detailed description!

> I believe the end result is not any stricter than before because the parse
> function (which is the caller in this case) strips white space before
> instantiating the CategoryFilter.  This is done in the  _parse_filter_flag
> function:

Ah, I see. I missed _parse_filter_flag is stripping the filters. So, I guess I
wanted a testcase for _parse_filter_flag about stripping. It may be like

	(files, options) = parse(['--filter= +foo , -bar '])
	self.assertEquals(options.filter_rules,
			  ['-', '+whitespace', '+foo', '-bar'])

But I don't think we should add this in this bug.

> > The BDFL [3] recommends inserting a blank line between the last paragraph
in a multi-line docstring and its closing quotes, placing the closing quotes on
a line by themselves. This way, Emacs' fill-paragraph command can be used on
it. ( from http://www.python.org/dev/peps/pep-0257/ )
> 
> Should we depart from this?  I don't care too much.  After having tried both
> though, I do think it looks a bit nicer with the space.  It looks less
> scrunched that way, especially if the subsequent code starts on the line
> immediately after the """.

Ah, I see. I didn't know this, thanks for the info! I don't have strong
preference on this and I guess other people don't care so much either.

> Yeah, I was doing that before, and then Eric recently told me that he and
Adam
> were leaning towards double quotes.  I don't really care as long as we're
> consistent.  If I had to make a choice, I'd probably prefer double because
it's
> more consistent with docstring quotes being double.  The Python style guide
> only seems to have an opinion about docstring quotes (which should be
double). 
> Should we try to settle on something with Eric, Adam, and David going
forward?

I have no strong preference here, too. Though having two styles in one file
isn't good, we may be able to fix all style inconsistencies issues in another
patch later. Let's go ahead.