Bug 36100 - check-webkit-style: Add support for a --debug option
Summary: check-webkit-style: Add support for a --debug option
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-14 13:48 PDT by Chris Jerdonek
Modified: 2010-03-24 00:53 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (18.13 KB, patch)
2010-03-20 16:15 PDT, Chris Jerdonek
hamaji: review+
hamaji: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-03-14 13:48:31 PDT
This should set the logging level to logging.DEBUG and adjust the log message format string as suggested in the FIXME.
Comment 1 Chris Jerdonek 2010-03-20 16:15:24 PDT
Created attachment 51236 [details]
Proposed patch
Comment 2 Shinichiro Hamaji 2010-03-23 18:41:07 PDT
Comment on attachment 51236 [details]
Proposed patch

The change looks good, but I think -v and --verbose would be the better naming as the name of the option to be consistent with other tools in WebKit. Also, in WebKit, --debug usually means "debug build" so I guess this use might be a bit confusing. Please feel free to land this patch if you like my proposal and change the name of the option.
Comment 3 Chris Jerdonek 2010-03-23 19:06:55 PDT
(In reply to comment #2)

Thanks much for reviewing!

> (From update of attachment 51236 [details])
> The change looks good, but I think -v and --verbose would be the better naming
> as the name of the option to be consistent with other tools in WebKit. Also, in
> WebKit, --debug usually means "debug build" so I guess this use might be a bit
> confusing. Please feel free to land this patch if you like my proposal and
> change the name of the option.

Yes, I had considered this.  The problem is that --verbose (which can be anything from 1 to 5) is already taken up as the option to throttle the style error level.  I considered using --verbose=0 to mean debug, but I think it is probably better not to use one option for two somewhat different purposes.

If we prefer --verbose for this, maybe we should rename the current --verbose usage to something else like --threshold (as in style threshold)?

Alternatively, we can swap names in a later patch so we don't do too much at once.

Let me know your preference.  Thanks again!
Comment 4 Chris Jerdonek 2010-03-23 19:11:26 PDT
(In reply to comment #3)
> If we prefer --verbose for this, maybe we should rename the current --verbose
> usage to something else like --threshold (as in style threshold)?

Or maybe --confidence would be a better name for the current option -- as suggested by how it is currently documented (i.e. minimum confidence level in the style error):

    verbose=#
      A number 1-5 that restricts output to errors with a confidence
      score at or above this value. In particular, the value 1 displays
      all errors. The default is %(default_verbosity)s.
Comment 5 Shinichiro Hamaji 2010-03-24 00:00:29 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > If we prefer --verbose for this, maybe we should rename the current --verbose
> > usage to something else like --threshold (as in style threshold)?
> 
> Or maybe --confidence would be a better name for the current option -- as
> suggested by how it is currently documented (i.e. minimum confidence level in
> the style error):
> 
>     verbose=#
>       A number 1-5 that restricts output to errors with a confidence
>       score at or above this value. In particular, the value 1 displays
>       all errors. The default is %(default_verbosity)s.

Thanks for the investigation! My 2 cents is that we change --verbose to --confidence and use --verbose for this issue. I guess current --verbose option isn't used well so this change won't make a lot of incompatibility issues. However, I can be convinced. Please wait for ~24 hours to see if someone disagree.

As for the order of these changes, I think it's OK to land this patch as-is, and then rename --debug and --verbose to --verbose and --confidence, respectively. However, it might be better to rename --verbose to --confidence first then land this patch.
Comment 6 Shinichiro Hamaji 2010-03-24 00:16:43 PDT
Comment on attachment 51236 [details]
Proposed patch

By the way, I've noticed the patch 6 on Bug 35498 (https://bugs.webkit.org/attachment.cgi?id=51236&action=prettypatch) contains a change which should be in this change.

> -    # Checking the debug flag before calling check_webkit_style_parser()
> -    # lets us enable more detailed logging earlier.
> +    # Checking for the debug flag before calling check_webkit_style_parser()
> +    # lets us enable debug logging earlier.
Comment 7 Chris Jerdonek 2010-03-24 00:20:46 PDT
(In reply to comment #5)
> Thanks for the investigation! My 2 cents is that we change --verbose to
> --confidence and use --verbose for this issue. I guess current --verbose option
> isn't used well so this change won't make a lot of incompatibility issues.
> However, I can be convinced. Please wait for ~24 hours to see if someone
> disagree.
> 
> As for the order of these changes, I think it's OK to land this patch as-is,
> and then rename --debug and --verbose to --verbose and --confidence,
> respectively. However, it might be better to rename --verbose to --confidence
> first then land this patch.

Thanks.  Your proposal sounds best.  I will go ahead and land this patch as-is.  If after 24 hours we don't hear any objections from anyone (e.g. from anyone currently relying on the --verbose flag), then I will rename --debug and --verbose to --verbose and --confidence, respectively, as you suggest.
Comment 8 Chris Jerdonek 2010-03-24 00:26:08 PDT
(In reply to comment #6)
> (From update of attachment 51236 [details])
> By the way, I've noticed the patch 6 on Bug 35498
> (https://bugs.webkit.org/attachment.cgi?id=51236&action=prettypatch) contains a
> change which should be in this change.
> 
> > -    # Checking the debug flag before calling check_webkit_style_parser()
> > -    # lets us enable more detailed logging earlier.
> > +    # Checking for the debug flag before calling check_webkit_style_parser()
> > +    # lets us enable debug logging earlier.

Excellent observation!  You must be in the process of reviewing that now -- thanks.  :)  I will go ahead and carry that change over before landing.

After landing this, I will rebase and re-attach the patch on bug 35498, which will have the added benefit of allowing the bots to apply that patch correctly in the bug report.  That's another reason for landing the current patch on this bug as-is (with that one change carried over of course).
Comment 9 Chris Jerdonek 2010-03-24 00:32:30 PDT
(In reply to comment #5)
> Thanks for the investigation! My 2 cents is that we change --verbose to
> --confidence and use --verbose for this issue. I guess current --verbose option
> isn't used well so this change won't make a lot of incompatibility issues.

By the way, I just searched WebKitTools/ for "--verbose" to see if anyone was perhaps invoking check-webkit-style internally using that option, and nothing relevant came up.  So we should at least be safe in that respect.
Comment 10 Chris Jerdonek 2010-03-24 00:53:21 PDT
Manually committed as discussed in comments 7 and 8:

http://trac.webkit.org/changeset/56437