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
Conversation
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.
LGTM % a nit
bbe9632
to
e958a22
Compare
@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, |
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.
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.
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.
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…
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.
"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.
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.
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.
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, |
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.
"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.
No description provided.