Make linter configurable in the package.json itself

[novemberborn]

This includes changes from #13, so see the last two commits:

Support a standard-engine key in the package.json which maps to a linter package. This will be the selected linter even if other linters are detected.

Support scoped packages, assuming their cmd is the package name without the scope prefix. Read options based on that unscoped package name.

For example:

{
  "standard-engine": "@novemberborn/as-i-preach",
  "as-i-preach": {
    "ignore": "path-to-ignore.js"
  }
}

Use resolve-from to resolve the linter module. This ensures the linter is indeed an installed package.

Ensure the worker is created with a CWD set to the projectRoot. Don't replace parts of pathToLinter, since this won't work reliably with scoped or globally installed linters.

---

The nice thing about this approach is standard-engine consumers can tell users to add this key to their package.json files and to use this Atom package. Then, magically, linting will work! IMO this means you could push this package as a more flexible replacement for linter-js-standard.

Using resolve-from lets us detect whether the linter is actually installed. Right now, if it isn't, the warning isn't surprised. Let me know if that should be changed.

resolve-from breaks the should be able to find a devDependency test since it skips the mocked file system. IMHO you could use actual fixtures instead and it'll will work. Or the resolve-from dependency will have to be mocked.

I find the test setup a bit daunting so I haven't tried to add tests for the new behavior yet. Please let me know if you like this approach!

[gustavnikolaj]

Great idea!

Support scoped packages, assuming their cmd is the package name without the scope prefix. Read options based on that unscoped package name.

I think this is a reasonable approach. :-)

The nice thing about this approach is standard-engine consumers can tell users to add this key to their package.json files and to use this Atom package. Then, magically, linting will work! IMO this means you could push this package as a more flexible replacement for linter-js-standard.

I agree. It's very much in line with (if not extending) the original intention of this plugin. I was annoyed with the existing plugins that would bundle specific versions of specific linters. I want the same errors to be triggered in CI and on all dev machines - not having it be a mess of "this worked fine in my editor!". This is further decoupling the plugin from the chosen configuration of the linter.

Using resolve-from lets us detect whether the linter is actually installed. Right now, if it isn't, the warning isn't surprised. Let me know if that should be changed.

As I noted above, I was intentional about keeping the plugin silent when it didn't find any linters. I would like the error only to be thrown if the user mentioned a linter in their project and it wasn't found.

If there is no standard-engine property in package.json and none of the supportedLinters are listed as devDependencies I would prefer it to stay silent :-)

resolve-from breaks the should be able to find a devDependency test since it skips the mocked file system. IMHO you could use actual fixtures instead and it'll will work. Or the resolve-from dependency will have to be mocked.

I'm okay with removing the mocked out fs if it get's in the way. It ought to work with require.resolve though - it's probably just a matter of the mocking running after the reference to require is taken by resolve-from. But I think I might have been carried away here - a normal fixture will do just fine :-)

I find the test setup a bit daunting so I haven't tried to add tests for the new behavior yet. Please let me know if you like this approach!

I very much like the approach! And if there's anything I can do to help you understand the test setup - or something unnecessary we can trim away (looking at you, mockfs! :eyes:) just let me know :-)

[gustavnikolaj]

I did some test refactoring on top of your work here: https://github.com/gustavnikolaj/linter-js-standard-engine/pull/15

Let me know how that looks to you.

[gustavnikolaj]

I invited you to be a collaborator, so if you have any additions, you can just push them on the PR i referenced. If you prefer not to, feel free to ignore the invitation :-)

[novemberborn]

I'm okay with removing the mocked out fs if it get's in the way. It ought to work with require.resolve though

resolve-from uses internal Node APIs for resolving the module, which I imagine skip the fs mocks. require.resolve won't work since it resolves relative to this package, not the package.json.

And if there's anything I can do to help you understand the test setup - or something unnecessary we can trim away (looking at you, mockfs! 👀) just let me know :-)

I think it's mostly the fixture setup. I'd prefer looking at a directory structure instead of code that generates that structure.

I did some test refactoring on top of your work here: #15

I'll have a look at that now.

I invited you to be a collaborator, so if you have any additions, you can just push them on the PR i referenced.

Thanks! Closing this in favor of #15.