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

Support setting flags globally and locally using cosmiconfig #354

Merged
merged 11 commits into from
Mar 31, 2019
Merged

Support setting flags globally and locally using cosmiconfig #354

merged 11 commits into from
Mar 31, 2019

Conversation

itaisteinherz
Copy link
Collaborator

Fixes #279.

// cc @voxpelli Let me know what you think and if you have any suggestions on how to improve this.

@sindresorhus

This comment has been minimized.

source/cli.js Outdated Show resolved Hide resolved
Copy link
Contributor

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, do we need to document it somewhere?

@itaisteinherz
Copy link
Collaborator Author

itaisteinherz commented Feb 28, 2019

do we need to document it somewhere?

This should be documented in the readme, in its own section.

@itaisteinherz
Copy link
Collaborator Author

@sindresorhus I addressed #279 (comment) in 67fb219.

@sindresorhus
Copy link
Owner

The code looks good, but haven't tested it yet. Can you add the docs?

@sindresorhus
Copy link
Owner

I think we should allow .np-config.js too.

@itaisteinherz
Copy link
Collaborator Author

Can you add the docs?

Sure

I think we should allow .np-config.js too.

Done

@sindresorhus
Copy link
Owner

We could really use some tests for this.

source/config.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@itaisteinherz
Copy link
Collaborator Author

itaisteinherz commented Mar 21, 2019

We could really use some tests for this.

I'm not really sure how this should be tested. For local configs, I guess we could create some fixtures to test out the behavior, but I have no idea how to mock is-installed-globally to test global configs. I suppose we could use something like proxyquire / mockery / rewiremock.

readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I just feel like we add more and more functionality and we have basically no tests.

@itaisteinherz
Copy link
Collaborator Author

I just feel like we add more and more functionality and we have basically no tests.

It bothers me as well. I think that this could be improved by:

  1. Splitting Add more tests #29 into smaller, more manageable, specific and actionable issues (e.g. Add tests for Git tasks).
  2. Allocating IssueHunt bounties to each issue (in case those are available to you, of course).

I'll also try to work on adding more tests (and add tests here as well).

@itaisteinherz
Copy link
Collaborator Author

@sindresorhus Also see #354 (comment).

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 31, 2019

Splitting #29 into smaller, more manageable, specific and actionable issues (e.g. Add tests for Git tasks).

👍 Would you be up for doing that?

Allocating IssueHunt bounties to each issue (in case those are available to you, of course).

Yup. Can do.

@sindresorhus
Copy link
Owner

I'll also try to work on adding more tests (and add tests here as well).

Can you open an issue or follow-up PR about that?

@sindresorhus sindresorhus merged commit abe7b02 into sindresorhus:master Mar 31, 2019
@sindresorhus
Copy link
Owner

Manually tested this and works well.

sindresorhus added a commit that referenced this pull request Mar 31, 2019
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@itaisteinherz itaisteinherz deleted the add-cosmiconfig branch March 31, 2019 16:52
@itaisteinherz
Copy link
Collaborator Author

👍 Would you be up for doing that?

Sure (will probably get it done in the next few days).

Can you open an issue or follow-up PR about that?

👌🏻

This was referenced Mar 31, 2019
@itaisteinherz
Copy link
Collaborator Author

@sindresorhus It would be great if you could reward me on IssueHunt for this and #334 🙃

@itaisteinherz itaisteinherz mentioned this pull request Apr 5, 2019
@itaisteinherz
Copy link
Collaborator Author

itaisteinherz commented Apr 5, 2019

Also, it seems like this breaks using $ np and $ np [increment]. @sindresorhus Can you verify? This seems like a quite severe issue. I'll try to get it fixed ASAP.

Edit: I also get a triple increment when using $ np [increment] (e.g. running $ np patch when current version is v4.0.0 releases v4.0.3 - see itaisteinherz/itai-playground@4def1f0 and https://npm.im/itai-playground). Probably unrelated to this though.

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

Successfully merging this pull request may close these issues.

None yet

3 participants