Update: Resolve npm installed formatters (#5900) #9464

testower · 2017-10-16T17:33:51Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
Attempt to solve issue:5900.

CLIEngine#getFormatter will check if format is resolvable as eslint-formatter-{format}. Providing the eslint-formatter- prefix in the format argument is optional. I have added unit test coverage for cli engine and documentation for the --format / -f has been updated.

Is there anything you'd like reviewers to focus on?
This is my first pull request in eslint. Be gentle :)

eslintbot · 2017-10-16T17:33:53Z

LGTM

eslintbot · 2017-10-17T07:45:52Z

LGTM

testower · 2017-10-17T13:02:39Z

Not sure what happened to continuous-integration/appveyor/pr, it passed on the previous run, and the only change is the commit message (added issue number).

platinumazure · 2017-10-17T15:22:47Z

Closing/reopening PR to restart CI.

testower · 2017-10-18T13:48:22Z

@platinumazure Thanks! Is there anything particular I need to do to initiate a review of this PR?

platinumazure · 2017-10-18T14:01:20Z

@testower Nah, the team will get to it soon.

@eslint/eslint-team Anyone want to review this?

btmills · 2017-10-19T13:10:36Z

lib/cli-engine.js

@@ -670,15 +673,21 @@ class CLIEngine {
            // replace \ with / for Windows compatibility
            format = format.replace(/\\/g, "/");

+            const cwd = this.options ? this.options.cwd : process.cwd();
+
            let formatterPath;

            // if there's a slash, then it's a file


This assumption doesn’t hold if I want to use a formatter from a scoped package on npm:

npm install @btmills/eslint-formatter-turtles eslint -f @btmills/turtles ...

This raises two questions:

Do we want to allow formatters from scoped packages?

If I use a formatted in a scoped package, can I omit the eslint-formatter- prefix?

If the answer to both questions is yes, then we might be able to borrow the code that resolves plugins and shared configs, which handles those already. I’d find a link if I weren’t on my phone.

Thanks. Seems like a reasonable requirement to me. Let me know if you find that link.

eslint/lib/config/plugins.js

Lines 40 to 77 in 1a962e8

/**

* Removes the prefix `eslint-plugin-` from a plugin name.

* @param {string} pluginName The name of the plugin which may have the prefix.

* @returns {string} The name of the plugin without prefix.

*/

static removePrefix(pluginName) {

return pluginName.startsWith(PLUGIN_NAME_PREFIX) ? pluginName.slice(PLUGIN_NAME_PREFIX.length) : pluginName;

}

/**

* Gets the scope (namespace) of a plugin.

* @param {string} pluginName The name of the plugin which may have the prefix.

* @returns {string} The name of the plugins namepace if it has one.

*/

static getNamespace(pluginName) {

return pluginName.match(NAMESPACE_REGEX) ? pluginName.match(NAMESPACE_REGEX)[0] : "";

}

/**

* Removes the namespace from a plugin name.

* @param {string} pluginName The name of the plugin which may have the prefix.

* @returns {string} The name of the plugin without the namespace.

*/

static removeNamespace(pluginName) {

return pluginName.replace(NAMESPACE_REGEX, "");

}

/**

* Defines a plugin with a given name rather than loading from disk.

* @param {string} pluginName The name of the plugin to load.

* @param {Object} plugin The plugin object.

* @returns {void}

*/

define(pluginName, plugin) {

const pluginNamespace = Plugins.getNamespace(pluginName),

pluginNameWithoutNamespace = Plugins.removeNamespace(pluginName),

pluginNameWithoutPrefix = Plugins.removePrefix(pluginNameWithoutNamespace),

shortName = pluginNamespace + pluginNameWithoutPrefix;

I have added test cases for the points raised by btmills and made them pass. I have also moved some helpers into a new "naming" utility (perhaps it should be called "prefix(-ing)"? The getFormatter is getting a bit complex, so I would prefer some refactoring, and maybe moving some logic to helper(s). What do you think.

eslintbot · 2017-10-21T20:14:19Z

LGTM

eslintbot · 2017-10-21T20:19:00Z

LGTM

eslintbot · 2017-10-22T08:56:31Z

LGTM

eslintbot · 2017-10-22T09:11:49Z

LGTM

eslintbot · 2017-10-22T09:42:11Z

LGTM

gyandeeps · 2017-10-27T20:24:04Z

lib/cli-engine.js

+            const namespace = naming.getNamespaceFromTerm(format);
+
+            if (namespace) {
+                cwd = `${cwd}/${namespace}`;


What is this line doing, can you please explain the with a use case?

Please see comment I left above regarding the same.

ilyavolodin

Overall, great job! Left a couple of small comments. I think there's an issue with namespaced packages though.

ilyavolodin · 2017-10-27T20:23:43Z

lib/cli-engine.js

+            const namespace = naming.getNamespaceFromTerm(format);
+
+            if (namespace) {
+                cwd = `${cwd}/${namespace}`;


I'm not sure this line is correct. Namespace shouldn't affect path (as far as I know), it's just a way to point to a specific package in NPM repository.

It should change the working directory to the "scope" of the scoped package. E.g. the package @foo/bar will be in node_modules/@foo/bar, so cwd should be node_modules/@foo so that bar can be resolved.

It's possible that I'm confused about something here, but when I install scoped packages this does indeed affect the path.

Oh I see where I might have got confused. The module resolver should already be able to handle scoped packages, right? I only need to remove the namespace in order to enforce the prefix. The namespace should be added back when passing the format to the module resolver? That should simplify quite a bit I think.

I have now updated the PR in an attempt to untangle this confusion. Hope that helps?

ilyavolodin · 2017-10-27T20:24:39Z

lib/cli-engine.js

+
+                try {
+                    formatterPath = resolver.resolve(npmFormat, cwd);
+                } catch (_) {


Nit: Could you change _ to e? We use e everywhere in the codebase, just want to be consistent

ilyavolodin · 2017-10-27T20:27:08Z

lib/util/naming.js

@@ -0,0 +1,59 @@
+/**


Nice! Good refactoring. However, I think config/config-file.js can also be updated to use this.

Did you intend for this to happen within this current pull request?

Just to clarify the scope of your request, this is related to normalizePackageName?

It seems after looking more closely that normalizePackageName can probably be reused in some way to make the logic in getFormatter a little nicer. So moving normalizePackageName in some way to the naming util looks like a good option.

Ok I moved the normalizePackageName to the naming util altogether. It simplified the logic in getFormatter quite a bit.

platinumazure

Love the direction this is going in! Just left a couple of minor comments.

platinumazure · 2017-10-28T18:51:37Z

lib/util/naming.js

+ * @returns {string} The namepace of the term if it has one.
+ */
+function getNamespaceFromTerm(term) {
+    return term.match(NAMESPACE_REGEX) ? term.match(NAMESPACE_REGEX)[0] : "";


Would it be possible to avoid executing the regular expression twice by storing the match result in a local variable?

Thanks for your comments @platinumazure I have updated as per your suggestions and tweaked some of the jsdoc / comments further, since the normalizaPackageName now applies not only to plugins and configs, but also to formatters.

platinumazure · 2017-10-28T18:53:22Z

lib/util/naming.js

@@ -0,0 +1,110 @@
+/**
+ * @fileoverview Common helpers naming of plugins and formatters


Nitpick: Seems to be missing a word here, maybe something like "Common helpers for naming of plugins and formatters"?

All issues have been fixed

ilyavolodin · 2017-11-01T21:43:59Z

Thanks for PR @testower !

aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Oct 16, 2017

testower force-pushed the issue5900 branch from 77397d5 to 910e53a Compare October 17, 2017 07:45

testower changed the title ~~Update: Resolve npm installed formatters~~ Update: Resolve npm installed formatters (#5900) Oct 17, 2017

platinumazure closed this Oct 17, 2017

platinumazure reopened this Oct 17, 2017

btmills reviewed Oct 19, 2017

View reviewed changes

testower force-pushed the issue5900 branch from 910e53a to f8635ca Compare October 21, 2017 20:14

testower force-pushed the issue5900 branch from f8635ca to 2b23e6f Compare October 21, 2017 20:18

testower force-pushed the issue5900 branch from 2b23e6f to cd9d6a2 Compare October 22, 2017 08:56

testower force-pushed the issue5900 branch from cd9d6a2 to 905cc91 Compare October 22, 2017 09:11

testower force-pushed the issue5900 branch from 905cc91 to 6e47a26 Compare October 22, 2017 09:42

testower closed this Oct 22, 2017

testower reopened this Oct 22, 2017

gyandeeps reviewed Oct 27, 2017

View reviewed changes

ilyavolodin suggested changes Oct 27, 2017

View reviewed changes

testower force-pushed the issue5900 branch 2 times, most recently from aaa484f to db7073c Compare October 28, 2017 17:26

platinumazure previously requested changes Oct 28, 2017

View reviewed changes

Update: Resolve npm installed formatters (eslint#5900)

8e7fb12

testower force-pushed the issue5900 branch from db7073c to 8e7fb12 Compare October 28, 2017 19:57

testower closed this Oct 29, 2017

testower reopened this Oct 29, 2017

ilyavolodin approved these changes Nov 1, 2017

View reviewed changes

ilyavolodin merged commit 4d7d7ab into eslint:master Nov 1, 2017

not-an-aardvark mentioned this pull request Nov 3, 2017

Improve the UX of using an npm installed formatter #5900

Closed

eslint-deprecated bot locked and limited conversation to collaborators May 1, 2018

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 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Resolve npm installed formatters (#5900) #9464

Update: Resolve npm installed formatters (#5900) #9464

testower commented Oct 16, 2017 •

edited

eslintbot commented Oct 16, 2017

eslintbot commented Oct 17, 2017

testower commented Oct 17, 2017

platinumazure commented Oct 17, 2017

testower commented Oct 18, 2017

platinumazure commented Oct 18, 2017

btmills Oct 19, 2017 •

edited

testower Oct 20, 2017

not-an-aardvark Oct 20, 2017

testower Oct 23, 2017

eslintbot commented Oct 21, 2017

eslintbot commented Oct 21, 2017

eslintbot commented Oct 22, 2017

eslintbot commented Oct 22, 2017

eslintbot commented Oct 22, 2017

gyandeeps Oct 27, 2017

testower Oct 27, 2017

ilyavolodin left a comment

ilyavolodin Oct 27, 2017

testower Oct 27, 2017 •

edited

testower Oct 27, 2017

testower Oct 28, 2017

ilyavolodin Oct 27, 2017

testower Oct 28, 2017

ilyavolodin Oct 27, 2017

testower Oct 28, 2017 •

edited

testower Oct 28, 2017

platinumazure left a comment

platinumazure Oct 28, 2017

testower Oct 28, 2017

platinumazure Oct 28, 2017

ilyavolodin commented Nov 1, 2017

	/**
	* Removes the prefix `eslint-plugin-` from a plugin name.
	* @param {string} pluginName The name of the plugin which may have the prefix.
	* @returns {string} The name of the plugin without prefix.
	*/
	static removePrefix(pluginName) {
	return pluginName.startsWith(PLUGIN_NAME_PREFIX) ? pluginName.slice(PLUGIN_NAME_PREFIX.length) : pluginName;
	}

	/**
	* Gets the scope (namespace) of a plugin.
	* @param {string} pluginName The name of the plugin which may have the prefix.
	* @returns {string} The name of the plugins namepace if it has one.
	*/
	static getNamespace(pluginName) {
	return pluginName.match(NAMESPACE_REGEX) ? pluginName.match(NAMESPACE_REGEX)[0] : "";
	}

	/**
	* Removes the namespace from a plugin name.
	* @param {string} pluginName The name of the plugin which may have the prefix.
	* @returns {string} The name of the plugin without the namespace.
	*/
	static removeNamespace(pluginName) {
	return pluginName.replace(NAMESPACE_REGEX, "");
	}

	/**
	* Defines a plugin with a given name rather than loading from disk.
	* @param {string} pluginName The name of the plugin to load.
	* @param {Object} plugin The plugin object.
	* @returns {void}
	*/
	define(pluginName, plugin) {
	const pluginNamespace = Plugins.getNamespace(pluginName),
	pluginNameWithoutNamespace = Plugins.removeNamespace(pluginName),
	pluginNameWithoutPrefix = Plugins.removePrefix(pluginNameWithoutNamespace),
	shortName = pluginNamespace + pluginNameWithoutPrefix;

		@@ -0,0 +1,110 @@
		/**
		* @fileoverview Common helpers naming of plugins and formatters

Update: Resolve npm installed formatters (#5900) #9464

Update: Resolve npm installed formatters (#5900) #9464

Conversation

testower commented Oct 16, 2017 • edited

eslintbot commented Oct 16, 2017

eslintbot commented Oct 17, 2017

testower commented Oct 17, 2017

platinumazure commented Oct 17, 2017

testower commented Oct 18, 2017

platinumazure commented Oct 18, 2017

btmills Oct 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Oct 21, 2017

eslintbot commented Oct 21, 2017

eslintbot commented Oct 22, 2017

eslintbot commented Oct 22, 2017

eslintbot commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyavolodin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testower Oct 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testower Oct 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyavolodin commented Nov 1, 2017

testower commented Oct 16, 2017 •

edited

btmills Oct 19, 2017 •

edited

testower Oct 27, 2017 •

edited

testower Oct 28, 2017 •

edited