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 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
Collaborator

@iiroj 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

@SachinShekhar SachinShekhar commented Sep 15, 2020

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
Collaborator

@iiroj 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

@SachinShekhar SachinShekhar commented Sep 15, 2020

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
Collaborator

@iiroj iiroj commented Sep 15, 2020

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

@iiroj
Copy link
Collaborator

@iiroj 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

@SachinShekhar SachinShekhar commented Sep 15, 2020

Done.

@iiroj
Copy link
Collaborator

@iiroj iiroj commented Sep 16, 2020

Thoughts, @okonet?

Copy link
Owner

@okonet okonet left a comment

It's LGTM but please consider documentation comments.

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

@iiroj iiroj left a comment

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
Co-authored-by: Iiro Jäppinen <iiro@jappinen.fi>
@SachinShekhar SachinShekhar force-pushed the SachinShekhar:function-config branch from 43cd672 to 9a8d245 Sep 16, 2020
@iiroj
iiroj approved these changes Sep 16, 2020
@okonet okonet merged commit 67a4d06 into okonet:master Sep 16, 2020
11 checks passed
11 checks passed
Node.js v10 on ubuntu-latest
Details
Node.js v10 on macos-latest
Details
Node.js v12 on ubuntu-latest
Details
Node.js v12 on macos-latest
Details
Node.js v14 on ubuntu-latest
Details
Node.js v14 on macos-latest
Details
Codecov
Details
Release
Details
codecov/patch Codecov Report
Details
codecov/project Codecov Report
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@github-actions
Copy link

@github-actions github-actions bot commented Sep 16, 2020

🎉 This PR is included in version 10.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released label Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.