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

feat: split tasks into chunks to support shells with limited max argument length #732

Merged
merged 3 commits into from Nov 20, 2019

Conversation

iiroj
Copy link
Member

@iiroj iiroj commented Nov 17, 2019

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 length maxLength, and the number of chunks is calculated as Math.min(Math.ceil(filesArg/maxLength), files.length). So when the filesArg is for example 3.5 times the length of maxLength, 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 be Running tasks (1/4)... and so on.

Fixes #147

@iiroj iiroj added this to the v10 milestone Nov 17, 2019
@codecov
Copy link

codecov bot commented Nov 17, 2019

Codecov Report

Merging #732 into beta will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/runAll.js 100% <100%> (ø) ⬆️
lib/chunkFiles.js 100% <100%> (ø)
lib/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b66cf3...feecb9d. Read the comment docs.

@iiroj
Copy link
Member Author

iiroj commented Nov 17, 2019

What do you think, @okonet?

@iiroj iiroj requested a review from okonet November 17, 2019 11:11
bin/lint-staged Outdated Show resolved Hide resolved
bin/lint-staged Outdated
lintStaged({
configPath: cmdline.config,
maxArgLength: MAX_ARG_LENGTH,
Copy link
Collaborator

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?

Copy link
Member Author

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.

lib/runAll.js Outdated Show resolved Hide resolved
@okonet
Copy link
Collaborator

okonet commented Nov 19, 2019

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?

@iiroj
Copy link
Member Author

iiroj commented Nov 20, 2019

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.

@okonet
Copy link
Collaborator

okonet commented Nov 20, 2019

Okay I’m convinced now. Let’s get it out!

Copy link
Collaborator

@okonet okonet left a 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?

@iiroj
Copy link
Member Author

iiroj commented Nov 20, 2019

Deleted test was for the warning, which no longer exists. There are new tests for the chunking function, and one for runAll where it runs two chunks: https://github.com/okonet/lint-staged/pull/732/files#diff-850db98aaae9bd6f45e2f5cbae2b9199R542

@okonet
Copy link
Collaborator

okonet commented Nov 20, 2019

Okay call sgtm

@iiroj iiroj merged commit cb43872 into beta Nov 20, 2019
@iiroj iiroj deleted the chunked-runner branch November 20, 2019 09:56
@iiroj
Copy link
Member Author

iiroj commented Nov 20, 2019

cool, merged this into beta, so it should get a new beta release soon.

@okonet
Copy link
Collaborator

okonet commented Nov 20, 2019

🎉 This PR is included in version 10.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@okonet
Copy link
Collaborator

okonet commented Jan 19, 2020

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants