Use getConfigForFile to handle configurations #1068

Arcanemagus · 2018-01-07T11:10:12Z

Move to using CLIEngine#getConfigForFile to handle determination of the configuration for a file instead of our attempts to replicate this behavior.

Note that if a user has a configuration in their home directory, and nowhere more specific for a file, previously we were counting this as "no configuration" for the purposes of the "Disable when no config"
option. After this change we are doing the same thing ESLint does: Using this configuration. If this isn't what a user desires, they should move away from a "global config".

Note: This function was introduced in ESLint v0.9.0, so this change will make us incompatible with versions older than that, however I believe we make more assumptions that require at least ESLint v1 elsewhere in the code so this isn't a huge deal.

Arcanemagus · 2018-01-07T11:10:51Z

I honestly expected this to be a larger change, while I was fixing the specs up I cleaned out several folders that were no longer used.

Arcanemagus · 2018-01-07T11:14:50Z

Ironically enough this is a return to using this API, as it was originally written for linter-eslint after a request by @jfaissolle in eslint/eslint#1309.

skylize · 2018-01-17T17:24:23Z

spec/worker-helpers-spec.js

-      const tempFixturePath = await copyFileToTempDir(fixtureFile)
-      const tempDir = Path.dirname(tempFixturePath)
-      const filepath = Path.join(tempDir, 'good.js')
+      const filePath = await copyFileToTempDir(fixtureFile)


I'm generally very much in favor of trying to shorten variable names to reduce noise, but this seems to end up way too vague.

The name filePath seems to denote a generic file or directory, this is a very specific file.

I think both "file" and "path" are fairly strongly implied by the usage and context of this variable. It takes almost 0 mental overhead to infer this is a filepath string, no matter what name we use.

With that in mind, my preference here would actually just be fixture. But I'm certainly fine also with tempFixture or even reverting to the old tempFixturePath.

tempFixturePath isn't the "old" name. If you look at what the code was doing previously it was getting the dirname of the tempFixturePath and then re-appending good.js onto it into filepath for some reason. I just cut out that busywork and am storing it directly into filePath.

As for filePath vs tempFixturePath, I'm not entirely sure what filePath would refer to other than a specific file's path? The idea with the current naming is that the code would look the same whether the file was temporary or not so I don't really like the idea of having temp in the name.

Do you do not think it is signficant the the path points to a fixture vs any other random file?

All of the paths point to a fixture and not "any other random file", so it would be quite significant if it didn't.

Ok fair enough. I'm still not loving it. Doesn't seem to mesh well with the way I think, but after hearing your arguments I'm down to about 1/10 on the GaS scale.

skylize · 2018-01-17T17:31:16Z

src/main.js

@@ -359,7 +332,7 @@ module.exports = {
        filePath,
        projectPath
      })
-      if (!isSave) {
+      if (!isSave && response) {


In what cases would would response have a falsey value here?

I think that I was only looking at the return here and somehow ignored the emit right above it. I don't think there is a case where that could be falsey.

skylize · 2018-01-17T18:00:11Z

src/worker-helpers.js

-    return configFile
+export function getConfigForFile(eslint, filePath) {
+  const cli = new eslint.CLIEngine()
+  try {


Is this a dangerous function call that could throw? No use of try in api docs.

If it can't find a configuration at all it throws an Error (hence the comment).

Arcanemagus · 2018-01-17T20:21:19Z

Note that if a user has a configuration in their home directory, and nowhere more specific for a file, previously we were counting this as "no configuration" for the purposes of the "Disable when no config"
option. After this change we are doing the same thing ESLint does: Using this configuration. If this isn't what a user desires, they should move away from a "global config".

I really want to draw attention to this as it's a change in behavior from what we currently do. I think it's a good idea since it brings us perfectly in line with how ESLint works, but I want to make sure people are thinking about it.

Move to using `CLIEngine#getConfigForFile` to handle determination of the configuration for a file instead of our attempts to replicate this behavior. Note that if a user has a configuration in their home directory, and nowhere more specific for a file, previously we were counting this as "no configuration" for the purposes of the "Disable when no config" option. After this change we are doing the same thing ESLint does: Using this configuration. If this isn't what a user desires, they should move away from a "global config". Co-Authored-By: Landon Abney <[email protected]>

aminya · 2021-03-14T06:54:45Z

For the record, this was rebased. A backup branch is available here:
https://github.com/AtomLinter/linter-eslint/tree/getConfigForFile-Backup

aminya · 2021-03-14T06:57:05Z

I am going to use this branch for a while to see if I see an issue with it. After a week or so, we can merge it.

This PR will help us with the following PR:
#1360

UziTech

LGTM

aminya

Awesome!

It is not as breaking as I thought. It works without any issues

github-actions · 2021-03-14T07:47:21Z

🎉 This PR is included in version 8.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Arcanemagus added the enhancement label Jan 7, 2018

Arcanemagus self-assigned this Jan 7, 2018

Arcanemagus requested a review from IanVS January 7, 2018 11:10

Arcanemagus force-pushed the arcanemagus/getConfigForFile branch 2 times, most recently from 65fd503 to a0c91ea Compare January 17, 2018 17:19

skylize suggested changes Jan 17, 2018

View reviewed changes

Arcanemagus force-pushed the arcanemagus/getConfigForFile branch from a0c91ea to 3cb0d85 Compare January 17, 2018 19:59

skylize approved these changes Jan 17, 2018

View reviewed changes

Arcanemagus force-pushed the arcanemagus/getConfigForFile branch 7 times, most recently from 9809c39 to 9c6a0da Compare January 23, 2018 17:33

Arcanemagus force-pushed the arcanemagus/getConfigForFile branch from 9c6a0da to 0aa336d Compare January 29, 2018 19:27

Arcanemagus force-pushed the arcanemagus/getConfigForFile branch from 0aa336d to 5928dc8 Compare March 2, 2018 21:22

IanVS mentioned this pull request Oct 16, 2018

Error: Cannot find module 'babel-eslint' #1186

Closed

IanVS mentioned this pull request Nov 12, 2018

ESlint not showing any errors/warnings #1198

Closed

aminya mentioned this pull request Mar 14, 2021

Update "eslint" dependency #1360

Closed

aminya added the enhancement label Mar 14, 2021

aminya force-pushed the arcanemagus/getConfigForFile branch from 6b97ed4 to 03685db Compare March 14, 2021 06:52

aminya force-pushed the arcanemagus/getConfigForFile branch from 03685db to fe8adf0 Compare March 14, 2021 06:54

aminya requested review from aminya and UziTech March 14, 2021 06:56

UziTech approved these changes Mar 14, 2021

View reviewed changes

aminya approved these changes Mar 14, 2021

View reviewed changes

aminya merged commit 5c79196 into master Mar 14, 2021

aminya deleted the arcanemagus/getConfigForFile branch March 14, 2021 07:43

github-actions bot added the released label Mar 14, 2021

This was referenced Mar 14, 2021

./src/*.js files are not linted in the Atom repository due to its non-conventional folder structure #1414

Closed

.eslintrc.cjs not checked with Disabling option #1379

Closed

UziTech mentioned this pull request Mar 17, 2021

.conf/eslint/eslintrc.js ignored after update #1418

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getConfigForFile to handle configurations #1068

Use getConfigForFile to handle configurations #1068

Arcanemagus commented Jan 7, 2018 •

edited

Loading

Arcanemagus commented Jan 7, 2018

Arcanemagus commented Jan 7, 2018

skylize Jan 17, 2018

Arcanemagus Jan 17, 2018

skylize Jan 17, 2018

Arcanemagus Jan 17, 2018

skylize Jan 17, 2018

skylize Jan 17, 2018

Arcanemagus Jan 17, 2018

skylize Jan 17, 2018

Arcanemagus Jan 17, 2018

Arcanemagus commented Jan 17, 2018

aminya commented Mar 14, 2021

aminya commented Mar 14, 2021 •

edited

Loading

UziTech left a comment

aminya left a comment

github-actions bot commented Mar 14, 2021

Use getConfigForFile to handle configurations #1068

Use getConfigForFile to handle configurations #1068

Conversation

Arcanemagus commented Jan 7, 2018 • edited Loading

Arcanemagus commented Jan 7, 2018

Arcanemagus commented Jan 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus commented Jan 17, 2018

aminya commented Mar 14, 2021

aminya commented Mar 14, 2021 • edited Loading

UziTech left a comment

Choose a reason for hiding this comment

aminya left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2021

Arcanemagus commented Jan 7, 2018 •

edited

Loading

aminya commented Mar 14, 2021 •

edited

Loading