Bug 33620 – check-webkit-style: Move error() from cpp_style.py to checker.py

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

This is part of the FIXME to move more code from cpp_style.py to checker.py, where appropriate.

[Chris Jerdonek]

Created attachment 46538 [details]
Proposed patch

[Chris Jerdonek]

(In reply to comment #1)
> Created an attachment (id=46538) [details]
> Proposed patch

I noticed that we can also now remove this line from cpp_style_unittest.py:

> from checker import CategoryFilter

I'll do that after the review.

[Shinichiro Hamaji]

Comment on attachment 46538 [details]
Proposed patch

Looks a sane code migration.

> -    def __init__(self, output_format, verbosity=1, filter=None,
> +    def __init__(self, output_format="emacs", verbosity=1, filter=None,

I think we can utilize DEFAULT_OUTPUT_FORMAT and DEFAULT_VERBOSITY?

> +        if ((verbosity < 1) or (verbosity > 5)):

I think we usually don't have parentheses for the condition of if-statement, but I don't care. Please remove them if you have no preference about this.

> +        ProcessorOptions() # No ValueError: works

I slightly prefer to examine the constructed value if it has sane attributes.

> +        # Spurious white space in filter rules.
> +        (files, options) = parse(['--filter=+foo ,-bar'])
> +        self.assertEquals(options.filter,
> +                          CategoryFilter(["-", "+whitespace", "+foo", "-bar"]))

Thanks for adding this!

[Chris Jerdonek]

(In reply to comment #3)

Thanks for the quick review, Shinichiro!  Submitting patch for cq shortly.

> > -    def __init__(self, output_format, verbosity=1, filter=None,
> > +    def __init__(self, output_format="emacs", verbosity=1, filter=None,
> 
> I think we can utilize DEFAULT_OUTPUT_FORMAT and DEFAULT_VERBOSITY?

The default parameter values above are meant more for convenience and internal use (like unit testing) rather than to be substantive.  That is why, for example, the default filter here should be the empty rule rather than the global variable list of filter rules for check-webkit-style.  Maybe in a future patch I can rename the DEFAULT values to something like SCRIPT_DEFAULT_OUTPUT_FORMAT to make clearer the distinction that those global variable defaults are for use by check-webkit-style.

> 
> > +        if ((verbosity < 1) or (verbosity > 5)):
> 
> I think we usually don't have parentheses for the condition of if-statement,
> but I don't care. Please remove them if you have no preference about this.

Thanks for the catch.  I also made this same change in a second place in the file.

> 
> > +        ProcessorOptions() # No ValueError: works
> 
> I slightly prefer to examine the constructed value if it has sane attributes.

Good suggestion.  Done.

[Chris Jerdonek]

Created attachment 46543 [details]
Proposed patch 2

[Shinichiro Hamaji]

Comment on attachment 46543 [details]
Proposed patch 2

Thanks for the description and fixing issues. I've found you fixed s/self.handle_error/self._handle_error/ . Thanks for catching this issue and sorry I didn't notice it. I think this means we want StyleCheckerTest later. Could you work for this? Otherwise, I'll write it.

[Chris Jerdonek]

(In reply to comment #6)
> (From update of attachment 46543 [details])
> Thanks for the description and fixing issues. I've found you fixed
> s/self.handle_error/self._handle_error/ . Thanks for catching this issue and
> sorry I didn't notice it. I think this means we want StyleCheckerTest later.
> Could you work for this? Otherwise, I'll write it.

I wasn't planning on doing it soon.  If you could do it, that would be great.  Thanks.

[WebKit Commit Bot]

Comment on attachment 46543 [details]
Proposed patch 2

Clearing flags on attachment: 46543

Committed r53251: <http://trac.webkit.org/changeset/53251>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.