-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Functions in testfiles.py should honor --repo-root #48210
base: master
Are you sure you want to change the base?
Conversation
More changes are still needed. Mark as draft for now. |
--repo-root is a parameter for wpt lint. As wpt tooling and tests are sitting in different dictories in Chromium, files_changed should accept repo_root as an arg to be consistent.
3147e42
to
22c2c54
Compare
This change is ready for review. @jgraham @jonathan-j-lee PTAL. Thanks |
def files_changed(repo_root: Text, | ||
revish: Text, | ||
ignore_rules: Optional[Sequence[Text]] = None, | ||
include_uncommitted: bool = False, | ||
include_new: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
def files_changed(repo_root: Text, | |
revish: Text, | |
ignore_rules: Optional[Sequence[Text]] = None, | |
include_uncommitted: bool = False, | |
include_new: bool = False | |
def files_changed(revish: Text, | |
ignore_rules: Optional[Sequence[Text]] = None, | |
include_uncommitted: bool = False, | |
include_new: bool = False, | |
repo_root: Text = wpt_root, |
Then there's no need to update the run_changed_files()
, run_tests_affected()
, wpt/run.py
, and ci/jobs.py
callsites.
Also, then there's no need for the repo_root or wpt_root
expression below, since the caller can just omit files_changed(repo_root=...)
.
@@ -133,7 +133,10 @@ def compile_ignore_rule(rule: Text) -> Pattern[Text]: | |||
return re.compile("^%s$" % "/".join(re_parts)) | |||
|
|||
|
|||
def repo_files_changed(revish: Text, include_uncommitted: bool = False, include_new: bool = False) -> Set[Text]: | |||
def repo_files_changed(wpt_root: Text, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming nit: How about repo_root
so that it doesn't shadow the global wpt_root
? Conceptually, it also seems more accurate.
--repo-root is a parameter for wpt lint. As wpt tooling and tests are sitting in different dictories in Chromium, files_changed should accept repo_root as an arg to be consistent.