Ignore home config from disableWhenNoEslintConfig #778
2 participants
| + atom.config.set('linter-eslint.disableWhenNoEslintConfig', true) | ||
| + | ||
| + waitsForPromise(() => | ||
| + new Promise((resolve) => { |
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); |
| @@ -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 |
I'm personally not a fan of one line if statements, but I'll leave that up to you.
| @@ -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' |
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
|
@Arcanemagus I believe I've addressed your feedback. |
Whichever way you decide to go with this, this LGTM otherwise so feel free to merge afterwards!
| @@ -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) |
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
I passed it in so that I could rimraf it after the test. Guess I could indeed rename it.
@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