Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the UX of using an npm installed formatter #5900

Closed
sindresorhus opened this issue Apr 19, 2016 · 9 comments
Closed

Improve the UX of using an npm installed formatter #5900

sindresorhus opened this issue Apr 19, 2016 · 9 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint help wanted The team would welcome a contribution from the community for this issue

Comments

@sindresorhus
Copy link
Contributor

This is currently what you have to write to use a custom npm installed formatter with ESLint:

$ eslint --format=node_modules/eslint-formatter-unicorn file.js

That's not very user-friendly.

I propose the following:

  1. Make getFormatter() try to require the formatter from cwd first. That way you could just do:
$ eslint --format=eslint-formatter-unicorn file.js
  1. Make it possible to omit the eslint-formatter prefix. This matches the convention for shareable configs. This combined with 1. would mean you could just do:
$ eslint --format=unicorn file.js

Much nicer! ✨

I would also document the recommendation of using the eslintformatter keyword in package.json, like you do with shareable configs. This would make it easier for users to find third-party formatters.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Apr 19, 2016
@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 19, 2016
@platinumazure
Copy link
Member

@sindresorhus To be clear, you are looking for this enhancement in CLIEngine and the CLI interface, if possible?

@sindresorhus
Copy link
Contributor Author

@platinumazure Yes, more specifically, in CLIEngine.getFormatter.

@ilyavolodin
Copy link
Member

I was wondering if we should instead add formatters to be distributed through plugins? Formatters are the only thing we currently have that we don't have a recommended way of shearing.

@nzakas
Copy link
Member

nzakas commented Apr 19, 2016

I fear this is more complicated than it looks. That said, @sindresorhus if you want to volunteer to investigate feasibility, we can consider it.

@sindresorhus
Copy link
Contributor Author

I've already looked into it, and doesn't seem that hard to me, unless I'm missing something.

I would just use resolve-from here and prepend eslint-formatter if missing.

Untested patch:

@@ -1,3 +1,5 @@
+var resolveFrom = require("resolve-from");
+
 CLIEngine.getFormatter = function(format) {

     var formatterPath;
@@ -11,13 +13,20 @@
         // replace \ with / for Windows compatibility
         format = format.replace(/\\/g, "/");

+        var cwd = this.options ? this.options.cwd : process.cwd();
+
         // if there's a slash, then it's a file
         if (format.indexOf("/") > -1) {
-            var cwd = this.options ? this.options.cwd : process.cwd();

             formatterPath = path.resolve(cwd, format);
         } else {
-            formatterPath = "./formatters/" + format;
+            var npmFormat = /^eslint-formatter-/.test(format) ? format : "eslint-formatter-" + format;
+
+            formatterPath = resolveFrom(cwd, npmFormat);
+
+            if (!formatterPath) {
+                formatterPath = "./formatters/" + format;
+            }
         }

         try {

@nzakas
Copy link
Member

nzakas commented Apr 21, 2016

I'd prefer not to add a second way to resolve modules (we already have a utility for that), but otherwise looks okay.

The next step would be to submit a pull request for the change. We'd need tests as well as documentation in order to merge.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion help wanted The team would welcome a contribution from the community for this issue and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 21, 2016
@nzakas nzakas removed the help wanted The team would welcome a contribution from the community for this issue label Sep 29, 2016
@kaicataldo
Copy link
Member

As per our discussion in today's TSC meeting, this issue has been added to the Core Roadmap.

@kaicataldo kaicataldo added the help wanted The team would welcome a contribution from the community for this issue label Jan 8, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 17, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 21, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 21, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 22, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 22, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 22, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 28, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 28, 2017
testower pushed a commit to testower/eslint that referenced this issue Oct 28, 2017
@testower
Copy link
Contributor

I forgot to mention, but I'm working on this one. It's in review.

@not-an-aardvark
Copy link
Member

This was added in #9464.

@not-an-aardvark not-an-aardvark moved this from Needs Design to Done in Core Roadmap Nov 3, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 3, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint help wanted The team would welcome a contribution from the community for this issue
Projects
No open projects
Development

No branches or pull requests

8 participants