Ignore home config from disableWhenNoEslintConfig #778
Conversation
IanVS
force-pushed
the
issue773-ignore-home-config
branch
from
291dc5c to
e3c5d6a
Compare
| atom.config.set('linter-eslint.disableWhenNoEslintConfig', true) | ||
|
|
||
| waitsForPromise(() => | ||
| new Promise((resolve) => { |
There was a problem hiding this comment.
Since this functionality is being used twice maybe it should be split into a function?
| * @return {Boolean} [description] | ||
| */ | ||
| export function isConfigAtHomeRoot(configPath) { | ||
| console.log(userHome, dirname(configPath); |
There was a problem hiding this comment.
Looks like this debug statement got left in 😛.
| @@ -54,7 +54,8 @@ module.exports = { | |||
| // Do not try to fix if linting should be disabled | |||
| const fileDir = Path.dirname(filePath) | |||
| const configPath = getConfigPath(fileDir) | |||
| if (configPath === null && disableWhenNoEslintConfig) return | |||
| const noProjectConfig = (configPath === null || isConfigAtHomeRoot(configPath)) | |||
| if (noProjectConfig && disableWhenNoEslintConfig) return | |||
There was a problem hiding this comment.
I'm personally not a fan of one line if statements, but I'll leave that up to you.
There was a problem hiding this comment.
That's how it was before, so I'm just going to leave it for now. :)
| @@ -6,6 +6,7 @@ import Path from 'path' | |||
| import { create } from 'process-communication' | |||
| import { FindCache, findCached } from 'atom-linter' | |||
| import * as Helpers from './worker-helpers' | |||
| import { isConfigAtHomeRoot } from './helpers' | |||
There was a problem hiding this comment.
helpers brings in a lot of things, and isn't intended to be used from the worker process (gotta love our convoluted design here...). Maybe this function should live in worker-helpers and helpers imports it from there? Either way is messy 😕.
It needs to be used in both worker and non-worker, so it’s easiest to just put it in its own file for now until we re-structure.
|
@Arcanemagus I believe I've addressed your feedback. |
| @@ -367,24 +380,14 @@ describe('The eslint provider for Linter', () => { | |||
| let editor | |||
| let didError | |||
| let gotLintingErrors | |||
| let tempFixtureDir | |||
| let tempFixturePath | |||
| const tempFixtureDir = fs.mkdtempSync(tmpdir() + path.sep) | |||
There was a problem hiding this comment.
Since this is the exact same in both test cases, you could move it into copyFileToTempDir, if you want to leave it as a parameter though you might as well just call the function copyFileToDir 😛.
There was a problem hiding this comment.
I passed it in so that I could rimraf it after the test. Guess I could indeed rename it.
There was a problem hiding this comment.
@Arcanemagus I ended up moving it to the function after all. Seemed … cleaner, I guess.
Fixes #773
Description
This PR prevents an eslint config file located in a user's home directory (e.g.
/users/person/.eslintrc) from enabling linting of projects within the home directory when the settingDisable when no ESLint config is foundis enabled.Note, because of the way we traverse up the directory structure looking for config files, projects outside of a user's home do not currently find a
fallbackconfig in a user's home, and thus those projects are already being disabled correctly.Notes
This adds a dependency on
user-home, which is used to detect the user's home directory and return it as a string. This result is compared to the directory containing the config file returned from the config file search, to determine if the file should be treated as a valid project config file or a fallback.Also, I've added a few specs here around the
disableWhenNoEslintConfigsetting, but I wasn't able to create a good red-green test for the particular fix in this PR. That's because I could not find a way to mock the functionality offindCachedin such a way to fake the finding of a config in the home directory. My plan was to put a file with linting errors in the OS temp dir, (not under the home dir), mock outfindCachedto make it seem there was a config file in the user home, and then show that with this PR, linting is still disabled. However, my go-to tool for such purposes,proxyquiredoes not seem to work correctly with electron'srequireimplementation. If you know if any other way to mock dependencies in the way I'm attempting, I'd be keen to hear it.How to test
Despite not having a good automated test for this change, it's fairly easy to check manually.
.jsfile similar to thebadInline.jsfile added in this pr, somewhere in your home directory, like~/experiment/foo.js.touch ~/.eslintrcdisableWhenNoEslintConfighas no effect, and linting is performed on the file no matter the setting.disableWhenNoEslintConfigis turned on.Warning
The base branch of this PR is not master, but rather another branch with an open PR. I have a good feeling that other PR (#777) will be merged, and rather than rebase this branch afterwards, I've just based on that, and will move the base to master after #777 is merged.
This change is