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 option to configure standard via rc config file #194

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TillaTheHun0
Copy link

Thanks for Standard! ❤️

This adds the ability to configure the StandardJS engine outside of package.json, ie. in a file like .standardrc, .standardrc.js, etc. Config in package.json is still prioritized over those options though, so no current behavior is changed. This removes a dep on pkg-conf and adds a dep on cosmiconfig.

From cosmiconfig README:

By default, Cosmiconfig will start where you tell it to start and search up the directory tree for the following:

- a `package.json` property
- a JSON or YAML, extensionless "rc file"
- an "rc file" with the extensions `.json`, `.yaml`, `.yml`, or `.js`.
- a `.config.js` CommonJS module

For example, if your module's name is "soursocks", cosmiconfig will search up the directory tree for configuration in the following places:

- a `soursocks` property in `package.json`
- a `.soursocksrc` file in JSON or YAML format
- a `.soursocksrc.json` file
- a `.soursocksrc.yaml`, `.soursocksrc.yml`, or `.soursocksrc.js` file
- a `soursocks.config.js` file exporting a JS object

I personally prefer tooling configuration outside of package.json bc then package.json will change only when a dep or script is updated, basically; easier to spot in a PR. Also makes it easier to spot when tooling config changes in a PR as well.

Also added a test for it as well.

Let me know what else it would need and thanks again!

@samhatoum
Copy link

This is very useful as it means our package.json only changes when deps change instead of also configs. Any view on this PR?

@TillaTheHun0
Copy link
Author

Any view on this? I see that standardx allows for a similar thing, via the eslintrc file, but I think this would be a nice thing to have built into vanilla standard.

@jasonkarns
Copy link
Contributor

I like this idea since I assume the primary goal is to allow a .standardrc within the project directory? Is there an implied feature that ~/.standardrc would be read as well? I'd be 👎 on that based on my (admittedly brief) skim through cosmiconfig's docs. It seems that cosmiconfig supports a searchPlaces option which would allow us to respect the XDG basedir spec. Without this, we would be removing users' ability to control how they want their configuration stored.

That said, if we can define searchPlaces in this PR to be XDG-compliant, then I'm on board!

@TillaTheHun0
Copy link
Author

@jasonkarns correct, that was my idea. I configure effectively all of my other tooling using dedicated config files except standard bc it currently doesn't support it. I could technically just use eslint directly and use the standard plugins in an .eslintrc.*, but that seems to cut against what standard is trying to accomplish; something more out of the box would be nice.

You make an interesting point. Admittedly, I am not familiar with the XDG basedir spec, besides just skimming it, but definitely familiar with some of the problems that it's trying to address 😄. I see some tools I use, like gatsby and verdaccio, are adhering to the relevant portion of the spec.

ESLint currently uses ~/.eslintrc.* if no configuration file is found in the project. This will be deprecated starting in v7, and removed in v8.

Aligning with ESLint and how it scans for a config file seems more kosher for this project IMO. That being said, if a maintainer thinks otherwise, perhaps merging #214, i'll be happy to make the changes that you suggested.

@TillaTheHun0
Copy link
Author

TillaTheHun0 commented Mar 25, 2020

It looks like #213 fixes those failing tests that are also failing on master. Perhaps, when that PR is merged, this and #214 can be rebased onto that. 👍

@TillaTheHun0 TillaTheHun0 force-pushed the master branch 3 times, most recently from e28ab8a to d22f7df Compare May 2, 2020 07:15
@theoludwig
Copy link
Member

Good idea, unfortunately there are conflicts with master branch!
Once conflicts are solved, we could review that PR and probably merge that to master!
Thanks for your contribution! @TillaTheHun0

@TillaTheHun0
Copy link
Author

Thanks for the followup @divlo . I can rebase and fix the conflicts. Though I need to make some changes to compliment #214 w.r.t @jasonkarns comment. Looks like cosmiconfig doesn't respect XDG (cosmiconfig/cosmiconfig#152), so I am currently trying to put together a PR there that will hopefully be merged. Then we could use that here.

@TillaTheHun0
Copy link
Author

@divlo I was able to figure out how to fallback to XDG config base dir without the PR to cosmiconfig. Though I probably will still try to PR there.

Basically what happens is standard-engine does the regular search for a config file, up to the user's home dir. If a config isn't found, then it will check XDG_CONFIG_HOME/${cmd} for a config* file. I also added a test for this.

I think this is ready to go. Let me know if it needs anything else!

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Great job! Unfortunately, the Travis CI checks failed. Maybe because you used var at some places.
Could you tell us, what would need to change once your changes on cosmiconfig is released ?

Also maybe we could add explanation about that feature in the README.

test/api.js Outdated Show resolved Hide resolved
@TillaTheHun0
Copy link
Author

I fixed the standard style, and added a section to the README. Let me know if any changes are needed.

@TillaTheHun0
Copy link
Author

Hmm, looks like the test for XDG is failing on CI. The tests work on my local, so this is odd. 🤔

@theoludwig
Copy link
Member

theoludwig commented Dec 13, 2020

Hmm, looks like the test for XDG is failing on CI. The tests work on my local, so this is odd.

It seems like the tests are also failing on my local machine. (I'm on Ubuntu 20.04 LTS)
Capture2

@TillaTheHun0
Copy link
Author

I wasn't clearing the require cache properly in the test, but it was using my own XDG_CONFIG_HOME so still passing 😅 . Should be fixed now!

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

4 participants