Skip to content
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

Add a -j argument to lint to control number of parallel jobs #25798

Merged
merged 1 commit into from Oct 7, 2020

Conversation

gsnedders
Copy link
Member

No description provided.

Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % a nit

tools/lint/lint.py Outdated Show resolved Hide resolved
@gsnedders
Copy link
Member Author

@Hexcles lmk if the new help string is better

@@ -957,6 +957,8 @@ def create_parser():
"working directory, not just files that changed")
parser.add_argument("--github-checks-text-file", type=ensure_text,
help="Path to GitHub checks output file for Taskcluster runs")
parser.add_argument("-j", "--jobs", type=int, default=0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably it's possible to pass in e.g. -1 as a value here.

In general I think I'd prefer to use None to represent the missing value and range check the argument. But that's not a blocker if you'd prefer not to change things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a number of existing tools that take 0 as a value that auto-detects. Certainly we can make this default to None and override that with 0 ourselves, but that seems silly…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"that's not a blocker if you'd prefer not to change things"

I just don't really like this kind of API which used magic numbers because at some point someone wrote some C code where the variable had type int and didn't have the option to do anything other than stuff additional meaning into out-of-range values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO there should be some way to access the default while still passing --jobs, and I'd much rather we matched what other software does rather than introduce a new way.

@Hexcles
Copy link
Member

Hexcles commented Sep 28, 2020

Yeah, looks better now, thanks!

@@ -957,6 +957,8 @@ def create_parser():
"working directory, not just files that changed")
parser.add_argument("--github-checks-text-file", type=ensure_text,
help="Path to GitHub checks output file for Taskcluster runs")
parser.add_argument("-j", "--jobs", type=int, default=0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"that's not a blocker if you'd prefer not to change things"

I just don't really like this kind of API which used magic numbers because at some point someone wrote some C code where the variable had type int and didn't have the option to do anything other than stuff additional meaning into out-of-range values.

@gsnedders gsnedders merged commit 98049d9 into web-platform-tests:master Oct 7, 2020
@gsnedders gsnedders deleted the lint-jobs branch October 7, 2020 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants