Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

linter-eslint disabled when used global config in home directory. #729

Closed
NTMan opened this issue Oct 7, 2016 · 20 comments
Closed

linter-eslint disabled when used global config in home directory. #729

NTMan opened this issue Oct 7, 2016 · 20 comments

Comments

@NTMan
Copy link

NTMan commented Oct 7, 2016

linter-eslint disabled when used global config in home directory.

Here possible using workaround if unchecked option "Disable when no ESLint config is found (in package.json or .eslintrc)" but expected that plugin would know about config in home directory.

eslint/eslint#3677 (comment)

@Arcanemagus
Copy link
Member

Arcanemagus commented Oct 7, 2016

This package follows the behavior outlined in the documentation, I'm not sure where that person got the idea that it will fallback to a file in the home directory. If it does, that's not documented.

If you want to replicate that behavior simply put that file's full path (/home/username/.eslintrc) into the ".eslintrc Path" setting and it will be used when a file can't be automatically found.

@NTMan
Copy link
Author

NTMan commented Oct 7, 2016

This behaviour described in documentation here: http://eslint.org/docs/user-guide/configuring#configuration-cascading-and-hierarchy

Note: If you have a personal configuration file in your home directory (~/.eslintrc), it will only be used if no other configuration files are found.

@Arcanemagus
Copy link
Member

In the context of the rest of that quote, they are talking about projects under the home directory, which would be covered by the "search up in directories" rule. I'm trying to find the actual code since there is conflicting information here...

@Arcanemagus
Copy link
Member

Arcanemagus commented Oct 7, 2016

Looks like the code follows the comment in the issue, and part of the documentation, marking this as a bug.

@IanVS IanVS reopened this Oct 8, 2016
@IanVS
Copy link
Member

IanVS commented Oct 8, 2016

Right, if there's an .eslintrc in the home directory, ESLint will normally use that for any project anywhere. If linter-eslint doesn't, then that's a bug.

@OtterCode
Copy link

Not entirely sure if my issue is the same or different, but when I used relative paths in the linter-eslint configuration, it failed silently.

To be exact: ~/.eslintrc fails, but /home/user/.eslintrc succeeds. That's highly annoying, since it reduces the portability of configs, and it fails to say anything about the error.

@OtterCode
Copy link

At minimum, the settings panel should include a note mentioning that absolute paths are required.

@IanVS
Copy link
Member

IanVS commented Dec 3, 2016

So, to clarify the state of this issue, the proposed change seems to be for linter-eslint to treat an eslint config file in the user's home directory as satisfying the requirement for Disable when no ESLint config is found (in package.json or .eslintrc), causing linting to happen if that option is checked and the only config is in the home directory. Is that accurate?

@oleander
Copy link

oleander commented Dec 3, 2016

@IanVS Not entirely sure what you're referring to. Is everything outside the current project directory treated as satisfying the requirement for Disable when no ESLint config is found (in package.json or .eslintrc)? I.e; a config file found in ~ or /, but not the current project directory disables linter-eslint when Disable when no ESLint config is found (in package.json or .eslintrc) is checked . Is that correct?

@IanVS
Copy link
Member

IanVS commented Dec 4, 2016

linter-eslint has no concept of a "current project directory", so it will search up to root looking for a config.

If it doesn't find one, and the Disable when no ESLint config is found option is checked, then it does not do any linting.

I believe the proposed change is to also check the home directory for a config.

@oleander
Copy link

oleander commented Dec 4, 2016

@IanVS eslint already checks the users home directory :) See: https://github.com/eslint/eslint/blob/master/lib/config.js#L147 Like you said, it stats with the current directory and moves up towards root. Then, if nothing is found it tries the users home directory.

How about using the option Disable when no ESLint config is found... for when no config is found in the project directory? That would make it possible to disable the linter for a specific project by not using a .eslintrc file, even if a *global one exist. I understand no such feature exist today, but that would be a possible use case without having to disable eslint-linter in Atom settings panel.

*parent directories or home dir

@IanVS
Copy link
Member

IanVS commented Dec 4, 2016

@oleander Yes I know that is what ESLint does. But, there is no API for ESLint to tell us whether it found a config file for a given file, so we also crawl the filesystem ourselves looking for a config file. The bug is that we do not check the home directory like ESLint does.

It would be nice to have a feature like you suggest, but how would you suggest determining which folder is the "project root"? It isn't as simple as you might think, and we would inevitably get it wrong for some exotic folder structures. So, I'm not inclined to think it's a realistic thing to add.

Honestly you're best off avoiding "global" eslint configs in most cases. It's usually best to put your development dependencies within the projects themselves, specifying both the version of ESLint as well as the configuration. This makes it easier to collaborate with other developers, and can avoid some unexpected configuration cascading results.

@oleander
Copy link

oleander commented Dec 4, 2016

Something like this should work.

import {CLIEngine} from "eslint";
const cli = new CLIEngine({/* ... */});
const Config = require("eslint/lib/config");
const paths = new Config(cli.options).findLocalConfigFiles("/a/b/c.js");
/* All possible config files, i.e `.eslintrc.yaml` */
const {CONFIG_FILES} = require("eslint/lib/config/config-file");

Then use chokidar to watch existing and upcoming configs using your OS filesystem watcher.

chokidar.watch(possibleConfigFile, {/* ... */});

@oleander
Copy link

oleander commented Dec 4, 2016

Project root; atom.workspace.getPaths()

@IanVS
Copy link
Member

IanVS commented Dec 4, 2016

Project root; atom.workspace.getPaths()

That only works if the user has not opened an individual file or subfolder of the project. Perhaps a rare case, but it can happen, and personally I think changing whether a file is linted or not based on how the file was opened in the editor would be confusing and unexpected behavior.

@oleander
Copy link

oleander commented Dec 4, 2016

atom.project.onDidChangePaths(function(paths) {}); should work.

@oleander
Copy link

oleander commented Dec 4, 2016

@IanVS That was a bit mean, sorry.

The reason I know this is b/c I worked on the problem a few weeks back, due to old hardware. My hard drive couldn't for some reason manage the amount of I/O operations (at least that's what atom's profiler told me. I wanted so see if it was possible to move from fs pull to push and at the same time implement linters push Indie API.

Spent about 2 weeks researching ESLint, linter and this lib to figure out an alternative solution. It works really well and is considerable faster (on my old MBP '09), mainly as almost no I/O operations are performed during linting.

The new (?) pusher linter API also lets me "reload" linter errors whenever a config or .eslintignore changes, without having to check with the file system.

Currently running a fork of this project with these "features", all specs passing and some added. I haven't had the time to share any code yet, but I'm happy to do so if you and your team are interested.

Also decided to write a small testing framework on-top of Jasmine to simplify the current specs as I had difficulties understanding the ranges being used.

describe("linter", function() {
  it("handles the bad", function() {
    lint("files/bad.js", function(handle) {
      handle.forRule("semi", function(message) {
        expect(message.type).toBe("Error");
        expect(message.text).toBeNull();
        expect(message.html).toContain("href");
        expect(message.highlight).toEqual(";");
      });

      handle.forRule("no-undef", function(message) {
        expect(message.type).toBe("Error");
        expect(message.text).toBeNull();
        expect(message.html).toContain("href");
        expect(message.highlight).toEqual("foo");
      });
    });
  });
});

@IanVS
Copy link
Member

IanVS commented Dec 4, 2016

We've gotten a bit off-topic for this issue. Would you mind opening a new issue, @oleander, with all the details of what you're suggesting?

@oleander
Copy link

oleander commented Dec 5, 2016

@IanVS Sure! I'll create a pr later this week with my suggestions.

@IanVS
Copy link
Member

IanVS commented Jan 5, 2017

Closing this issue in preference to #773. Let's move any other discussion over to that issue.

@IanVS IanVS closed this as completed Jan 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants