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: add ability to use function as config #913

Merged
merged 5 commits into from
Sep 16, 2020

Conversation

SachinShekhar
Copy link
Contributor

@SachinShekhar SachinShekhar commented Sep 15, 2020

This PR adds an ability to use a function as config. This function will get an array of all staged files as parameter which can be used with own globs. Currently, we need to pass such function as value of '*' in config object which doesn't feel good.

This can be further optimized by not using micromatch internally when a function config is detected.

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #913 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #913   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        18    +1     
  Lines          602       608    +6     
  Branches       142       143    +1     
=========================================
+ Hits           602       608    +6     
Impacted Files Coverage Δ
lib/formatConfig.js 100.00% <100.00%> (ø)
lib/index.js 100.00% <100.00%> (ø)

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 643038d...9a8d245. Read the comment docs.

@iiroj
Copy link
Member

iiroj commented Sep 15, 2020

Thanks for the PR! I wonder if the formatConfig is needed, could the same thing simply be inside the validateConfig method as a short-circuit?

@SachinShekhar
Copy link
Contributor Author

Separation of Concerns

validateConfig should focus on only validation, nothing else.
Benefit of separate formatConfig: If you need to accept strings or arrays as config in the future, it'd be easy to implement thanks to formatConfig.

@iiroj
Copy link
Member

iiroj commented Sep 15, 2020

Good enough for me! Do you think there are some existing examples In the readme that could use this format instead of the start glob?

@SachinShekhar
Copy link
Contributor Author

There was one example and I have already added the new alternative (but, didn't remove existing one because that remains valid). Do you want me to remove that existing example?

@iiroj
Copy link
Member

iiroj commented Sep 15, 2020

Let's leave it, since it shows the explicit '*' glob.

@iiroj
Copy link
Member

iiroj commented Sep 15, 2020

I think the readme could be more clear that you can either export a single function that receives all matched files, or then an object of globs, where each key can be a function (that receives only those matches), string or array of strings.

Otherwise it's good to go IMO!

@SachinShekhar
Copy link
Contributor Author

Done.

@iiroj
Copy link
Member

iiroj commented Sep 16, 2020

Thoughts, @okonet?

okonet
okonet previously approved these changes Sep 16, 2020
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.

It's LGTM but please consider documentation comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@iiroj iiroj left a comment

Choose a reason for hiding this comment

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

I wrote some direct suggestions on the README text. What do you think?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@okonet okonet merged commit 67a4d06 into lint-staged:master Sep 16, 2020
@github-actions
Copy link
Contributor

🎉 This PR is included in version 10.4.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
Labels
Development

Successfully merging this pull request may close these issues.

3 participants