Summary: | check-webkit-style: Add support for a --debug option | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, cjerdonek, eric, hamaji, levin | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Chris Jerdonek
2010-03-14 13:48:31 PDT
Created attachment 51236 [details]
Proposed patch
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.
(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! (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. (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 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. (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. (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). (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. Manually committed as discussed in comments 7 and 8: http://trac.webkit.org/changeset/56437 |