-
-
Notifications
You must be signed in to change notification settings - Fork 426
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
feat: split tasks into chunks to support shells with limited max argument length #732
Conversation
Codecov Report
@@ Coverage Diff @@
## beta #732 +/- ##
==========================================
+ Coverage 99.13% 99.21% +0.07%
==========================================
Files 12 13 +1
Lines 346 381 +35
Branches 64 77 +13
==========================================
+ Hits 343 378 +35
Misses 3 3
Continue to review full report at Codecov.
|
475bd67
to
9abe995
Compare
What do you think, @okonet? |
bin/lint-staged
Outdated
lintStaged({ | ||
configPath: cmdline.config, | ||
maxArgLength: MAX_ARG_LENGTH, |
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.
Does this has to be an option? What purpose?
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.
It's here so it's only passed to the functions when run through the cli. Directly calling the functions leaves it undefined. Good catch, I should update the README anyway, and this affects the node API behaviour.
It was mainly done for tests.
9abe995
to
86b562f
Compare
Still so much more code to maintain that I’m not sure about that change. Do you think just having a check and throwing a warning wouldn’t suffice? |
86b562f
to
feecb9d
Compare
I don't think this is too complicated. The good thing compared to the previous chunking implementation is that this is completely separate from the generation of tasks, and will simply generate multiple "runs" of tasks from a smaller set of files. It is also run on all platforms, not just on Windows, so the implementation doesn't vary. I guess the warning wasn't good enough for Windows users, because any team with at least one Windows developer would always have to use function linters and basically implement this feature themselves. |
Okay I’m convinced now. Let’s get it out! |
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.
What about deleted tests? Did you substitute them?
Deleted test was for the warning, which no longer exists. There are new tests for the chunking function, and one for |
Okay call sgtm |
cool, merged this into beta, so it should get a new beta release soon. |
🎉 This PR is included in version 10.0.0-beta.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 10.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR adds chunking to running tasks to better support Windows shells with more limited maximum argument string length.
The logic of chunking is to first create the full argument string
filesArg
by joining the list of staged files (and resolving as absolute paths when necessary). This string's length is compared to a safe value of half of the maximum argument lengthmaxLength
, and the number of chunks is calculated asMath.min(Math.ceil(filesArg/maxLength), files.length)
. So when thefilesArg
is for example 3.5 times the length ofmaxLength
, four chunks are created. If there for whatever reason are less files than chunks, at most one file per chunk is created.After chunking, instead of a single
Running tasks...
, there will beRunning tasks (1/4)...
and so on.Fixes #147