New: add a defineParser function #9321

CompuIves · 2017-09-17T15:43:42Z

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

[x] 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)

I added an defineParser function to the Node API, which allows you to define a custom parser that eslint will use when defined in the config.

Is there anything you'd like reviewers to focus on?

I'm not sure if the implementation is desired, returning the error object for errors doesn't feel right. I'm also not sure if I added enough tests.

platinumazure

Left one question, thanks for contributing!

platinumazure · 2017-09-17T18:02:19Z

lib/parsers.js

+        const parser = this._parsers[parserId];
+
+        if (!parser) {
+            return require(parserId);


Should require(parserId) be cached into the parser map in this case?

Hmm, node already caches requires by default (https://nodejs.org/api/modules.html#modules_require_cache), but I could add it for explicitness.

not-an-aardvark

Thanks for the PR! I left a few suggestions.

not-an-aardvark · 2017-09-17T18:11:44Z

docs/developer-guide/nodejs-api.md

@@ -241,6 +241,26 @@ Map {
 */
 ```

+### Linter#defineParser
+
+Each instance of `Linter` holds a map of custom parsers. If you want to define a parser programmaticaly you can add this function


The word "programmatically" is spelled incorrectly here.

not-an-aardvark · 2017-09-17T18:15:08Z

docs/developer-guide/nodejs-api.md

+    }
+});
+
+const results = linter.verify("// some source text", { parser: 'my-custom-parser' });


Nitpick: For consistency, can you use double quotes for "my-custom-parser"?

not-an-aardvark · 2017-09-17T18:22:38Z

lib/parsers.js

+// Public Interface
+//------------------------------------------------------------------------------
+
+class Parsers {


I think having a separate module for this isn't necessary given that a regular Map basically does the same thing (aside from using require). Instead, I think this could just be included as part of the defineParser method. However, I don't really have a strong objection to doing it like this.

Agreed. This feels like a pretty generic store - I think it would actually be clearer to use a Map as well.

not-an-aardvark · 2017-09-17T18:23:16Z

lib/linter.js

@@ -727,6 +709,7 @@ module.exports = class Linter {
        this.version = pkg.version;

        this.rules = new Rules();
+        this.parsers = new Parsers();


Can you prefix this property name with an underscore? (this._parsers)

not-an-aardvark · 2017-09-17T18:23:39Z

lib/linter.js

+     * @param {any} parserModule The parser object
+     * @returns {void}
+     */
+    defineParser(parserId, parserModule) {


Can you add tests for defineParser?

eslintbot · 2017-09-28T17:52:00Z

LGTM

not-an-aardvark

This looks good to me aside from a few documentation nitpicks. Thanks!

not-an-aardvark · 2017-09-29T00:35:23Z

docs/developer-guide/nodejs-api.md

+### Linter#defineParser
+
+Each instance of `Linter` holds a map of custom parsers. If you want to define a parser programmatically you can add this function
+with the name of the parser as first argument and the parser object (having either `parseForESLint` or `parse` as keys) as second argument.


I think it would be good to link to the parser documentation here. For example:

...with the name of the parser as the first argument and the [parser object](/docs/developer-guide/working-with-plugins#working-with-custom-parsers) as the second argument.

not-an-aardvark · 2017-09-29T00:35:46Z

docs/developer-guide/nodejs-api.md

+Each instance of `Linter` holds a map of custom parsers. If you want to define a parser programmatically you can add this function
+with the name of the parser as first argument and the parser object (having either `parseForESLint` or `parse` as keys) as second argument.
+
+If during linting the parser is not found it will fallback to `require(parserId)`.


Nitpick: I think a comma should be added after the word "found".

eslintbot · 2017-09-29T09:36:23Z

LGTM

CompuIves · 2017-09-29T09:36:44Z

Good feedback! I committed the suggested changes.

kaicataldo

Thanks for working on this!

kaicataldo · 2017-10-11T21:51:59Z

lib/parsers.js

+// Public Interface
+//------------------------------------------------------------------------------
+
+class Parsers {


Agreed. This feels like a pretty generic store - I think it would actually be clearer to use a Map as well.

kaicataldo · 2017-10-22T19:46:48Z

@CompuIves Friendly ping.

CompuIves · 2017-10-31T10:31:02Z

Sorry for the delayed reply, I got caught up in some work. I just pushed a commit that moves the _parsers property to a Map.

jsf-clabot · 2017-11-05T21:32:04Z

All committers have signed the CLA.

bruno12mota · 2017-11-23T16:18:35Z

@CompuIves great work on this! Would be awesome to merge this and get a release going since it's been some time and having this as a git dependency will collide with other eslint dep in some cases.

platinumazure

LGTM, thanks!

not-an-aardvark · 2017-11-26T02:54:35Z

Thanks for contributing!

Update: add a defineParser function

0601a24

CompuIves mentioned this pull request Sep 17, 2017

Update: Allow parsers to be defined as object #9119

Closed

platinumazure reviewed Sep 17, 2017

View reviewed changes

not-an-aardvark suggested changes Sep 17, 2017

View reviewed changes

not-an-aardvark 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 Sep 18, 2017

not-an-aardvark added this to In Progress in Core Roadmap Sep 19, 2017

Process PR feedback

17d09ae

not-an-aardvark approved these changes Sep 29, 2017

View reviewed changes

Update documentation

9428e74

kaicataldo reviewed Oct 11, 2017

View reviewed changes

Move parsers to a Map

e899c99

Chore: use assert.strictEqual

b893b0d

ilyavolodin approved these changes Nov 24, 2017

View reviewed changes

platinumazure approved these changes Nov 26, 2017

View reviewed changes

not-an-aardvark merged commit 28c9c8e into eslint:master Nov 26, 2017

not-an-aardvark moved this from In Progress to Done in Core Roadmap Jan 25, 2018

renovate bot mentioned this pull request Feb 14, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

not-an-aardvark mentioned this pull request Mar 23, 2018

Refactor parser handling out of Linter? #10118

Closed

eslint-deprecated bot locked and limited conversation to collaborators May 26, 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 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: add a defineParser function #9321

New: add a defineParser function #9321

CompuIves commented Sep 17, 2017 •

edited

platinumazure left a comment

platinumazure Sep 17, 2017

CompuIves Sep 17, 2017

not-an-aardvark left a comment

not-an-aardvark Sep 17, 2017

not-an-aardvark Sep 17, 2017

not-an-aardvark Sep 17, 2017

kaicataldo Oct 11, 2017

not-an-aardvark Sep 17, 2017

not-an-aardvark Sep 17, 2017

eslintbot commented Sep 28, 2017

not-an-aardvark left a comment

not-an-aardvark Sep 29, 2017

not-an-aardvark Sep 29, 2017

eslintbot commented Sep 29, 2017

CompuIves commented Sep 29, 2017

kaicataldo left a comment

kaicataldo Oct 11, 2017

kaicataldo commented Oct 22, 2017

CompuIves commented Oct 31, 2017 •

edited

jsf-clabot commented Nov 5, 2017 •

edited

bruno12mota commented Nov 23, 2017

platinumazure left a comment

not-an-aardvark commented Nov 26, 2017

New: add a defineParser function #9321

New: add a defineParser function #9321

Conversation

CompuIves commented Sep 17, 2017 • edited

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

not-an-aardvark 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 28, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 29, 2017

CompuIves commented Sep 29, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Oct 22, 2017

CompuIves commented Oct 31, 2017 • edited

jsf-clabot commented Nov 5, 2017 • edited

bruno12mota commented Nov 23, 2017

platinumazure left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Nov 26, 2017

CompuIves commented Sep 17, 2017 •

edited

CompuIves commented Oct 31, 2017 •

edited

jsf-clabot commented Nov 5, 2017 •

edited