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

Find XO config based on linted file path #425

Merged
merged 6 commits into from
Feb 17, 2020
Merged

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Feb 17, 2020

fix #331, fix #279, fix #330, fix #420, fix #65

This change the way to load the XO config files. Instead of looking for the config in directly cwd we now search for a config file starting from the directory of each linted and by walking up parent directories.

The config files supported are now: pacakge.json, .xorc (JSON only), .xorc.json, .xorc.js, xo.config.js

This PR will help to implement #373 as we can now build a different config per file mode easily. That will allow us to apply the Typescript rules only to the .ts/.tsx files.

A few other things incidentally added by the PR:

  • A way to override the default ignores #65 is now fixed
  • Workspaces Broken vscode-linter-xo#58 should be fixed without any changes in vscode-linter-xo
  • lintFiles is now completely async allowing to parallelise looking for config files (async) with building/merginf config (sync)
  • Reorganized the code a little bit for clarity as the files and function started to be quite large
  • Replace multimatch with micromatch as it's the library used by globby which is already in the dependency tree

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@pvdlg pvdlg requested a review from sindresorhus February 17, 2020 08:12
@sindresorhus
Copy link
Member

Yay! This is really awesome 🙌


The config files supported are now: pacakge.json, .xorc (JSON only), .xorc.json, .xorc.js, xo.config.js

Can we make it:

  • .xorc => .xo-config
  • .xorc.json => .xo-config.json

And maybe drop .xorc.js since there's already xo.config.js?

@sindresorhus
Copy link
Member

And maybe drop .xorc.js since there's already xo.config.js?

Hmm, I guess it can be useful if you want to hide it. But should be .xorc.js => .xo-config.js

@sindresorhus
Copy link
Member

#65 is now fixed

🎉 Could you add a short readme example to show the user that this is possible?

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 17, 2020

Can we make it:

  • .xorc => .xo-config
  • .xorc.json => .xo-config.json

That's really common among JS projects to have the config files being based on the package name (without a -config) for example .prettierrc or .eslintrc and not .prettier-config or .eslint-config. I think breaking from this convention would be confusing and error prone for users.

It would also create a discrepancy between the package.json property, the json files and the JS files.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 17, 2020

🎉 Could you add a short readme example to show the user that this is possible?

Added a Tips

@sindresorhus
Copy link
Member

That's really common among JS project to have the config files being based on the package name (without a -config)

And it's still based on the package name. We're just replacing rc with -config for clarity.

I just don't like the rc suffix. It only makes sense if you know what https://github.com/dominictarr/rc is, and I've been in Node.js for years and I don't really know what rc stands for. "resource configs"?. And by not having a separator, it's not always clear what it is. .xorc, reads to me as xorc, not xo rc. What if the package name was co. Then the file would be .corc.

I think that breaking from this convention would be confusing and error prone for users.

I honestly think the convention with rc is more confusing than not following the convention. xo-config makes it totally clear what it is. No questions asked. Imagine a newbie getting into JS, cloning a random project, and seeing a .xorc file without knowing what XO or rc is. Do you think they immediately realize it's config file?

It would also create a discrepancy between the package.json property, the json files and the JS files.

The package.json property is xo, the current filename is xorc. There's already a discrepancy. -config is not any more of a discrepancy than rc.

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 17, 2020

Made the change for config file name

@sindresorhus
Copy link
Member

There's a slight regression in the code coverage: https://coveralls.io/builds/28770195/source?filename=lib/options-manager.js#L261

Otherwise, this looks good to me. When merged, should I do a new release or do you plan other changes soon?

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 17, 2020

There's a slight regression in the code coverage: https://coveralls.io/builds/28770195/source?filename=lib/options-manager.js#L261

That was on line 303 before and wasn't tested either.
I improved the test coverage for the part I changes that changed

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 17, 2020

Otherwise, this looks good to me. When merged, should I do a new release or do you plan other changes soon?

I'm working on the Typescript support today. Not sure if I'll finish by the end of day. If I don't open the PR today, it will probably take a few days.

@pvdlg pvdlg force-pushed the find-conf-from-file branch from 031ec8b to 51dae6c Compare February 17, 2020 19:50
@sindresorhus
Copy link
Member

sindresorhus commented Feb 17, 2020

Weird, not sure why Coveralls said the coverage fell on that line then 🤷‍♂️

@sindresorhus
Copy link
Member

I'm working on the Typescript support today. Not sure if I'll finish by the end of day. If I don't open the PR today, it will probably take a few days.

Sounds good. Then I'll wait. Take your time. I was really just wondering whether anything would happen in the next few weeks for it to be worth waiting for.

@sindresorhus sindresorhus merged commit e0f81a7 into master Feb 17, 2020
@sindresorhus sindresorhus deleted the find-conf-from-file branch February 17, 2020 20:08
@sindresorhus
Copy link
Member

Thank you so much for working on this. This is such a great improvement 🙌

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.

A way to override the default ignores Support an external config file instead of just package.json config
2 participants