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

Allow config to be provided via command-line #304

Merged

Conversation

cameronhunter
Copy link
Contributor

@cameronhunter cameronhunter commented Oct 7, 2017

Summary

I use lint-staged as a pre-commit hook and would like to explicitly provide the config file location. This PR uses commander to parse the command-line argument and passes it as cosmiconfig's configPath option. This allows the config file to be any format already supported.

Test plan

I've added tests and updated the documentation.

@sudo-suhas
Copy link
Collaborator

@cameronhunter Could you investigate the CI failures?

@codecov
Copy link

codecov bot commented Oct 13, 2017

Codecov Report

Merging #304 into master will increase coverage by 4.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   91.86%   95.95%   +4.09%     
==========================================
  Files          10       10              
  Lines         172      173       +1     
  Branches       26       27       +1     
==========================================
+ Hits          158      166       +8     
+ Misses         13        7       -6     
+ Partials        1        0       -1
Impacted Files Coverage Δ
src/index.js 87.5% <100%> (+10.08%) ⬆️
src/runAll.js 92.59% <0%> (+14.81%) ⬆️

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 eacb3d2...49d83f5. Read the comment docs.

@cameronhunter
Copy link
Contributor Author

@sudo-suhas The CI failures were:

  1. Cross-platform difference in paths between Windows and Linux. Resolved using a jest serializer.
  2. In some CI setups Listr decides to be verbose and write more to the console, which meant the snapshots were inconsistent. Resolved by injecting the console logger instead of mocking the global console.

@sudo-suhas sudo-suhas mentioned this pull request Oct 14, 2017
@sudo-suhas
Copy link
Collaborator

sudo-suhas commented Oct 14, 2017

Hey @cameronhunter, thanks for this. I did not know about the jest snapshot serializer. Cool stuff.

Regarding 99d44b9, do you think it is possible that the whitespace issue was because the editor removed trailing whitespace? I personally have that enabled in vscode.

In some CI setups Listr decides to be verbose and write more to the console

Actually, I don't think this is the case. I am only guessing, but I think this has something to do with running the code in sync vs async. In all our current tests, since cosmiconfig was being mocked, we only captured the console logs which were printed by us. And after that listr wrote some more logs. However, in the test you added, cosmiconfig ran async and by the time we awaited for lintStaged, listr had already pushed some logs to console. For some reason, this isn't happening locally. Anyway, listr behavior isn't different but our way of capturing console logs is buggy. So I think it is the right approach to inject the logger.

However, I think a lot of complexity was added in the way it has been implemented now. I took some of your commits and tweaked them in #311. Could you please remove commits ce2c8d7, 327f1c7, 58b034a, 505654a, 40b399a, afe2fac and port the changes from d512c63 instead? You can use git reset to remove commits. Let me know if you need any help.

This branch is out-of-date with the base branch

Edit: If you could rebase this on master, that would be good too.

@cameronhunter cameronhunter force-pushed the allow_config_via_commandline branch 5 times, most recently from d55b3be to 7b6f85e Compare October 15, 2017 21:18
@cameronhunter
Copy link
Contributor Author

cameronhunter commented Oct 15, 2017

@sudo-suhas I've rebased on master, squashed to a single commit and used npm run cz for formatting the commit message.

To prevent a breaking change, I reordered the parameters to the main lib to pass the logger first (with a fallback value of console). Overall this feature addition should just be a minor version bump.

CI is passing – let me know if there's anything more you'd like included before merging.

@@ -0,0 +1,6 @@
{
"verbose": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change this to true? We'd be able to see the config in the snapshot then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Configuration file can be provided via a -c or --config commandline parameter. This allows users to
be explicit with where their configuration lives rather than relying on cosmiconfig path search.
Copy link
Collaborator

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

Great Work @cameronhunter! LGTM.

@cameronhunter
Copy link
Contributor Author

@sudo-suhas Can you speak to a timeline of merging and publishing the new version?

@sudo-suhas
Copy link
Collaborator

I figured I'd wait a couple of days for comments, if any, from @okonet and @luftywiranda13. I'll wait till end of day and merge it in. It should be released immediately via semantic-release.

@luftywiranda13
Copy link
Collaborator

is it better if we also add some additional helpful things for the --help page?

@sudo-suhas sudo-suhas merged commit 54809ae into lint-staged:master Oct 18, 2017
@sudo-suhas
Copy link
Collaborator

@luftywiranda13 I think that makes sense.

@sudo-suhas
Copy link
Collaborator

@cameronhunter This has been released in v4.3.0. Apologies for the delay and thank you for your contribution!

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

Successfully merging this pull request may close these issues.

3 participants