Bug 32592 – [check-webkit-style] Preparatory code moves to refactor cpp_style.parse_arguments

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

The function cpp_style.parse_arguments currently has side effects of setting global variables (and also writing to sys.stderr).

Refactor parse_arguments so this doesn't happen.  This will make unit testing easier and also make it more transparent when called from check-webkit-style.

I am working on this.

[Chris Jerdonek]

Created attachment 45227 [details]
Proposed patch

Let me know if you want me to split this into two patches.

It looks longer than it really is because of some large deletes.

In any case, I just want to get it out there so you can see it in one piece.  Thanks.

[Chris Jerdonek]

Created attachment 45290 [details]
Proposed patch 2

Brought up to date.

[Shinichiro Hamaji]

Comment on attachment 45290 [details]
Proposed patch 2

> Let me know if you want me to split this into two patches.
>
> It looks longer than it really is because of some large deletes.

Yeah, I think it would be better to split this patch. I'd suggest moving code chunk first, then refactoring.

> @@ -360,15 +310,15 @@ class _CppStyleState(object):
>      """Maintains module-wide state.."""
>  
>      def __init__(self):
> -        self.verbose_level = _DEFAULT_VERBOSITY  # global setting.
> +        self.verbose_level = 1  # global setting.
>          self.error_count = 0    # global count of reported errors
>          # filters to apply when emitting error messages
> -        self.filters = _DEFAULT_FILTER_RULES[:]
> +        self.filters = []
>  
>          # output format:
>          # "emacs" - format that emacs can parse (default)
>          # "vs7" - format that Microsoft Visual Studio 7 can parse
> -        self.output_format = _DEFAULT_OUTPUT_FORMAT
> +        self.output_format = 'emacs'
>  
>      def set_output_format(self, output_format):
>          """Sets the output format for errors."""

I'm not sure, but the default value of verbose_level, filters, and output_format must be always overwritten? If so, None would be better as the initial value? If you are planning to remove _CppStyleState, it's OK as is.

[Chris Jerdonek]

(In reply to comment #3)

> Yeah, I think it would be better to split this patch. I'd suggest moving code
> chunk first, then refactoring.

Thanks for the comments, Shinichiro.  Sure, I'll go ahead and do that.

> > @@ -360,15 +310,15 @@ class _CppStyleState(object):
> > -        self.filters = _DEFAULT_FILTER_RULES[:]
> > +        self.filters = []
> I'm not sure, but the default value of verbose_level, filters, and
> output_format must be always overwritten? If so, None would be better as the
> initial value? If you are planning to remove _CppStyleState, it's OK as is.

Yes, I was planning to remove those properties in a future patch.  I just didn't want it to look like the global variables were playing a role there.

[Chris Jerdonek]

Created attachment 45313 [details]
Proposed patch 3 (dividing earlier patch into two)

Dividing into two patches as Shinichiro requested.  This patch has all of the moves with the minimum changes needed for things to work.

[Chris Jerdonek]

Created attachment 45317 [details]
Proposed patch 4

I noticed that the defaults were not getting set after the code move, so I changed two lines of style.parse_arguments() to this:

    verbosity = _DEFAULT_VERBOSITY
    output_format = _DEFAULT_OUTPUT_FORMAT

I'm not sure how much this matters since it will be refactored away momentarily.

[Shinichiro Hamaji]

Comment on attachment 45313 [details]
Proposed patch 3 (dividing earlier patch into two)

Thanks for splitting the patch! Looks good assuming this patch is basically moving code.

> @@ -48,117 +47,9 @@ import sys
>  import unicodedata
>  
>  
> -# This is set by check-webkit-style.
>  _USAGE = ''

We don't need this in cpp_style anymore?

[Chris Jerdonek]

(In reply to comment #7)
> (From update of attachment 45313 [details])
> Thanks for splitting the patch! Looks good assuming this patch is basically
> moving code.

Yeah, pretty much.  The only changes within moves were selectively changing some "cpp_style." prefixes to "style." or "".

> >  _USAGE = ''
> We don't need this in cpp_style anymore?

You're right -- we don't.  I can get rid of it in part two if that's okay.

[David Levin]

Comment on attachment 45313 [details]
Proposed patch 3 (dividing earlier patch into two)

It appears that patch 4 replaces this one.

[Chris Jerdonek]

(In reply to comment #9)
> (From update of attachment 45313 [details])
> It appears that patch 4 replaces this one.

Thanks, David.  Does Bugzilla allow us to "obsolete" a patch without attaching a new one?  I couldn't find where to do this in the UI after forgetting to check the obsolete box.

[Eric Seidel (no email)]

You can click on the "details" for a patch to obsolete it.

If you have lots of patches to obsolete, then "bugzilla-tool obsolete-attachments BUGNUM" will obsolete all attachments on a bug for you.

[David Levin]

Eric said it. I should finish one this sometime today.

[David Levin]

Comment on attachment 45317 [details]
Proposed patch 4

> Index: ChangeLog

WebKitTools/ChangeLog....
This isn't going to land easily (using cq+) since the patch isn't rooted at WebKit.


> Index: Scripts/modules/cpp_style.py
> -# This is set by check-webkit-style.
>  _USAGE = ''

Is this _USAGE still needed?

[Chris Jerdonek]

(In reply to comment #13)

Thanks for the review, David.

> > Index: Scripts/modules/cpp_style.py
> > -# This is set by check-webkit-style.
> >  _USAGE = ''
> 
> Is this _USAGE still needed?

No, I will remove it in the second patch.

[Shinichiro Hamaji]

Committed r52541: <http://trac.webkit.org/changeset/52541>

[Shinichiro Hamaji]

Comment on attachment 45317 [details]
Proposed patch 4

It seems the ChangeLog had two issues:

- It didn't have "Reviewed by NOBODY (OOPS!)". Please consider using prepare-ChangeLog to add a ChangeLog entry.
- It had only two lines of context and patch command was confused. Normally, it should have 3 lines in the context. This also happened when I landed another patch you posted. I'm not sure why this happened.

[Chris Jerdonek]

(In reply to comment #16)
> (From update of attachment 45317 [details])
> It seems the ChangeLog had two issues:

Thank you for landing this patch, Shinichiro, and for correcting and reporting on the ChangeLog issues.

I do use prepare-Changelog.  I must have overwritten the "Reviewed by" line by accident.  I will be more attentive to that in the future.

I don't know why the context has only two lines at the end of that portion.  I tried re-running svn-create-patch and verified that it has only two lines.  In contrast, running "svn diff" does generate three lines of context at the end.  I will investigate further for a possible cause and, if necessary, file a bug report.  Thanks.

[Chris Jerdonek]

(In reply to comment #17)
> I will investigate further for a possible cause and, if necessary, file a bug
> report.  Thanks.

I created a bug report for this issue here:

https://bugs.webkit.org/show_bug.cgi?id=32919

[Chris Jerdonek]

(In reply to comment #15)
> Committed r52541: <http://trac.webkit.org/changeset/52541>

It looks like the commit did not include the following new file in the patch: Scripts/modules/style_unittest.py.  Also, the run-webkit-unittests scripts can't execute without this file.

[Daniel Bates]

Added missing file style_unittest.py and committed in <http://trac.webkit.org/changeset/52583>. Without this file, run-webkit-unittests died with the following error:

Traceback (most recent call last):
  File "WebKitTools/Scripts/run-webkit-unittests", line 49, in <module>
    from modules.style_unittest import *
ImportError: No module named style_unittest


(In reply to comment #19)
> (In reply to comment #15)
> > Committed r52541: <http://trac.webkit.org/changeset/52541>
> 
> It looks like the commit did not include the following new file in the patch:
> Scripts/modules/style_unittest.py.  Also, the run-webkit-unittests scripts
> can't execute without this file.

[Chris Jerdonek]

Moving part 2 to here:

https://bugs.webkit.org/show_bug.cgi?id=32966