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.

Sign up for GitHub

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

Jump to bottom

fix(rule-finder): pass a file to getConfigForFile #306

Closed
wants to merge 1 commit into from
Closed

fix(rule-finder): pass a file to getConfigForFile #306

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/rule-finder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function _getConfig(configFile) {
// Point to the particular config
configFile
});
return cliEngine.getConfigForFile();
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this only return the proper config for the config file, which might be yaml or .eslintrc json?

Config files are often not JS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps. Consider this more of a bug report than a PR then. I'm not sure what the proper solution for this is. Because for this project there's not really a specific file that we're looking for a config for 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

#304 is the bug report, which includes my latest attempt: master...ljharb:eslint_6

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦‍♂ I apologize. Totally didn't even think to check whether it had been reported. This is embarrassing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries, to be fair I was super excited when I saw the PR notification because this package's v6 support is blocking me in two shared configs :-/

Copy link

Choose a reason for hiding this comment

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

@ljharb (or anyone else looking for issues related to .js configs as I was and finding this comment)
re:

Config files often not JS.

by the way:
If you look at the top eslint configs on the first page of NPM, 12 out of the 18 use the .js config style (as opposed to .json or .yaml). The list of popular configs using this style includes:

(I already had this list)

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, but these are the top shared configs. I have 200 packages that only use the rc form, and a shared config for many of them that also does. Config files are indeed often not JS, even though they are also often JS :-)

return cliEngine.getConfigForFile(configFile);
}

function _getCurrentNamesRules(config) {
Expand Down