| Summary: | Add script to search for feature defines | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Don Olmstead <don.olmstead> | ||||||||
| Component: | Tools / Tests | Assignee: | Don Olmstead <don.olmstead> | ||||||||
| Status: | ASSIGNED --- | ||||||||||
| Severity: | Normal | CC: | ews-watchlist, glenn, jbedard, mcatanzaro | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Don Olmstead
2020-03-06 11:39:32 PST
Created attachment 392760 [details]
WIP Patch
Comment on attachment 392760 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392760&action=review > Tools/Scripts/find-feature-flags.py:1 > +#!/usr/bin/env python This should be Tools/Scripts/find-feature-flags Comment on attachment 392760 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392760&action=review > Tools/Scripts/find-feature-flags.py:37 > +_ROOT = os.path.normpath(os.path.join(os.path.dirname(__file__), "..", "..")) Since this is in Tools/Scripts, not much of a reason to mark things with _ in this file. We usually do that to indicate a variable should be considered private. > Tools/Scripts/find-feature-flags.py:67 > + parser.add_argument('find', help='the code to search', choices=_CHOICES, nargs='*') The help could be better. Maybe something like'The type of configuration file or code to search for feature flags in' > Tools/Scripts/find-feature-flags.py:68 > + parser.add_argument('--diff', choices=_CHOICES, action='append') In my experience, we sort of need to baby people with more powerful options like this, otherwise they will never get used. This deserves a help string. > Tools/Scripts/webkitpy/featuredefines/matcher.py:30 > +@total_ordering I'm pretty sure this working in Python 2 and 3, but could you run this script in both to be certain? > Tools/Scripts/webkitpy/featuredefines/matcher.py:87 > +def usage_matcher(macro='ENABLE'): I tend to dislike free functions in Python, these, in particular, seem to be good @classmethods for FeatureDefineMatcher (maybe just renamed to Matcher since the import path makes it clear?) > Tools/Scripts/webkitpy/featuredefines/matcher_unittest.py:1 > +# Copyright (C) 2020 Sony Interactive Entertainment You need an __init__.py in featuredefines. Created attachment 393059 [details]
WIP Patch
Updating with feedback
Created attachment 393111 [details]
WIP Patch
Fix some issues
Comment on attachment 393111 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393111&action=review > Tools/Scripts/find-feature-defines:1 > +#!/usr/bin/env python Make this script executable. > Tools/Scripts/find-feature-defines:33 > +_DESCRIPTION = ''' Make sure that we have a description before landing. > Tools/Scripts/find-feature-defines:37 > +_ROOT = os.path.normpath(os.path.join(os.path.dirname(__file__), "..", "..")) We usually don't use underscored variables in the script itself since underscores mark a variable as private and we wouldn't be importing from a script anyways. > Tools/Scripts/find-feature-defines:72 > + parser.add_argument('--diff', choices=_CHOICES, action='append') This need a better description |