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

Support for file loaders plug-ins #1817

Closed
wooorm opened this Issue Feb 10, 2015 · 28 comments

Comments

Projects
None yet
4 participants
@wooorm

wooorm commented Feb 10, 2015

Hey! First of all: thanks for the awesome tool. I use it every day and I’d say it makes my coding like so much easier :)

Second: I just launched eslint-md. A tool which functions just like eslint, but on the embedded code blocks in Markdown. During the creation, I found how little code was needed to add this support (props to the easy use of CLIEngine). Most of the code actually went into making eslint-md work exactly like eslint.

That made me think of adding support for plug-ins which accept certain file extensions.

A working theory: add an extensions hash next to rules and rulesConfig on the plugin object.

// functions just like `eslint.verify`
function markdown(text, config, filename, saveState) {
    var eslint = this;
    var javascript = processToJavaScript(text);
    var messages = eslint.verify(javascript, config, filename, saveState);
    fixPositionInMessages(messages);
    return messages;
}

module.exports = {
    extensions: {
        "markdown": markdown,
        "md": markdown
    }
};

There are some problems with extensions though, such as stdin support.

What do you think? Is ESLint the right place? Interested in implementing?

EDIT: Seems GFM does not like <stdin>.

@nzakas nzakas added the triage label Feb 10, 2015

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 10, 2015

Member

Inserting idea, I'm not opposed to this summer plugins are supposed to let you alter the behavior of ESLint. The way I'd envision this working would be for the plugin to produce a string with the extra non-js parts stripped out, so it before a file on reading the file rather than an all out replacement for verify().

Member

nzakas commented Feb 10, 2015

Inserting idea, I'm not opposed to this summer plugins are supposed to let you alter the behavior of ESLint. The way I'd envision this working would be for the plugin to produce a string with the extra non-js parts stripped out, so it before a file on reading the file rather than an all out replacement for verify().

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 10, 2015

Thanks for the reply, I’m not sure what “summer” has got to do with it though. 😉 Did you mean “since”?

I think I agree on not replacing eslint.verify itself, however, simply creating a JavaScript version of a document might not be sufficient enough: markdown, for example, can contain multiple JavaScript “files”.

Therefore, I’d argue that the preprocessing state could lead to multiple calls to verify, or return multiple “files”.

In addition, the postprocessing state would (might?) piece those reports back together, and fix the position in messages to correspond to the actual line/column in the input file.

P.S. I just found out my initial issue missed part of the second-last paragraph (about stdin).

wooorm commented Feb 10, 2015

Thanks for the reply, I’m not sure what “summer” has got to do with it though. 😉 Did you mean “since”?

I think I agree on not replacing eslint.verify itself, however, simply creating a JavaScript version of a document might not be sufficient enough: markdown, for example, can contain multiple JavaScript “files”.

Therefore, I’d argue that the preprocessing state could lead to multiple calls to verify, or return multiple “files”.

In addition, the postprocessing state would (might?) piece those reports back together, and fix the position in messages to correspond to the actual line/column in the input file.

P.S. I just found out my initial issue missed part of the second-last paragraph (about stdin).

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 10, 2015

Member

Damn autocorrect, sorry about that. I'm not sure I agree that markdown or anything else could have "multiple files". Ultimately, you want to be told the line number of an error in the file you're working on, it doesn't really matter how many sections are in that file, just that the error is on line 99. Why go through the trouble of splitting them up then trying to fix the line/col info later?

FWIW, JSHint does this for linting JavaScript inside of HTML files. It just blanks out everything that isn't JavaScript and lints the result. That also seems like the best part forward for markdown.

Member

nzakas commented Feb 10, 2015

Damn autocorrect, sorry about that. I'm not sure I agree that markdown or anything else could have "multiple files". Ultimately, you want to be told the line number of an error in the file you're working on, it doesn't really matter how many sections are in that file, just that the error is on line 99. Why go through the trouble of splitting them up then trying to fix the line/col info later?

FWIW, JSHint does this for linting JavaScript inside of HTML files. It just blanks out everything that isn't JavaScript and lints the result. That also seems like the best part forward for markdown.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 10, 2015

Member

Not linting blocks of JS separately might lead to strange errors and warnings, like no-redeclare for example. On the other hand, linting them separately might trigger rules such as no-use-before-define. Ideally, you should be able to tell plugin how to lint the file, maybe through comments, but that's implementation detail.

Member

ilyavolodin commented Feb 10, 2015

Not linting blocks of JS separately might lead to strange errors and warnings, like no-redeclare for example. On the other hand, linting them separately might trigger rules such as no-use-before-define. Ideally, you should be able to tell plugin how to lint the file, maybe through comments, but that's implementation detail.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 11, 2015

Member

Good point.

Member

nzakas commented Feb 11, 2015

Good point.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 11, 2015

Member

To expand on my previous comment, if you are making it into plugin architecture, then it should be up to plugin to determine if splitting a given file into multiple files is necessary. Then is should pass all files (or one file) content into eslint, and eslint should allow for error object to be fed back into the plugin so it could adjust error position information. In general I like @wooorm proposal of preprocessor and postprocessor, it feels like it would require minimal changes in eslint to implement, just a few additional hooks.

Member

ilyavolodin commented Feb 11, 2015

To expand on my previous comment, if you are making it into plugin architecture, then it should be up to plugin to determine if splitting a given file into multiple files is necessary. Then is should pass all files (or one file) content into eslint, and eslint should allow for error object to be fed back into the plugin so it could adjust error position information. In general I like @wooorm proposal of preprocessor and postprocessor, it feels like it would require minimal changes in eslint to implement, just a few additional hooks.

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 11, 2015

@nzakas I can see where you’re coming from: it seems like loads of trouble for a small possible gain.
Especially when going the “JSHint HTML” way (regexes). However, eslint-md traverses an AST (by mdast), containing code blocks. With a position object, a language: "js" flag to let me know I need to check it, and the raw javascript as a string. There’s no real concept of a file, and nothing to replace or comment out.

Of course, I could piece those code object together and treat them as a single file, or I could treat code blocks per section as a single file (read: scope). I argue for it, giving the plugin author that choice, for scalability to other use cases.

Re: preprocessor & postprocessor, what do you think?

@ilyavolodin I’ve ran eslint-md on several files, and found that both treating them as a single file, or every code block separately resulted in errors. But thats a markdown thing right: who uses “use strict” in code examples? So yeah, i agree, it should be a plugin’s choice.

wooorm commented Feb 11, 2015

@nzakas I can see where you’re coming from: it seems like loads of trouble for a small possible gain.
Especially when going the “JSHint HTML” way (regexes). However, eslint-md traverses an AST (by mdast), containing code blocks. With a position object, a language: "js" flag to let me know I need to check it, and the raw javascript as a string. There’s no real concept of a file, and nothing to replace or comment out.

Of course, I could piece those code object together and treat them as a single file, or I could treat code blocks per section as a single file (read: scope). I argue for it, giving the plugin author that choice, for scalability to other use cases.

Re: preprocessor & postprocessor, what do you think?

@ilyavolodin I’ve ran eslint-md on several files, and found that both treating them as a single file, or every code block separately resulted in errors. But thats a markdown thing right: who uses “use strict” in code examples? So yeah, i agree, it should be a plugin’s choice.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 11, 2015

Member

My main concern is having something that will work for more than just markdown. That doesn't mean we need to allow everything. It does mean the same tools should be able to work in multiple situations.

I don't want plugins to call verify() directly, so come up with a proposal that works fulfills that requirement, plus yours, plus linting js in HTML and then we can talk about it.

Member

nzakas commented Feb 11, 2015

My main concern is having something that will work for more than just markdown. That doesn't mean we need to allow everything. It does mean the same tools should be able to work in multiple situations.

I don't want plugins to call verify() directly, so come up with a proposal that works fulfills that requirement, plus yours, plus linting js in HTML and then we can talk about it.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 12, 2015

Member

Why would it need to call verify? It's a plugin, not a replacement for cli. All that would need to be done is changing processFile function to load plugins from config file, and before calling eslint.verify pass the file to appropriate plugin's preprocess method. If plugin returns array, loop through it and call verify for each. After that, pass messages to plugin's postprocess before returning them. This way, custom loaders can be added, like, for example, loader that would compile coffee script before running eslint on the result (not that it's a good example, but I'm sure somebody is going to write one at some point).
This would also require adding optional block into .eslintrc that would map extensions to loaders.

Member

ilyavolodin commented Feb 12, 2015

Why would it need to call verify? It's a plugin, not a replacement for cli. All that would need to be done is changing processFile function to load plugins from config file, and before calling eslint.verify pass the file to appropriate plugin's preprocess method. If plugin returns array, loop through it and call verify for each. After that, pass messages to plugin's postprocess before returning them. This way, custom loaders can be added, like, for example, loader that would compile coffee script before running eslint on the result (not that it's a good example, but I'm sure somebody is going to write one at some point).
This would also require adding optional block into .eslintrc that would map extensions to loaders.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 12, 2015

Member

The example in the description uses verify(), I was referring to that.

I think it's more complicated than you describe. How would an array be represented in output? One entry per array item? One entry per file? Right now we assume there's one result per file, we don't know where that assumption would break things. How would line numbers be reconciled in the output?

Still a lot that is unclear here.

Member

nzakas commented Feb 12, 2015

The example in the description uses verify(), I was referring to that.

I think it's more complicated than you describe. How would an array be represented in output? One entry per array item? One entry per file? Right now we assume there's one result per file, we don't know where that assumption would break things. How would line numbers be reconciled in the output?

Still a lot that is unclear here.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 12, 2015

Member

Sorry, wasn't clear, array wouldn't be represented in the output at all... Preprocessor will return an array of strings that would be then sent to verify function (one at a time). Output of the verify function will be sent to postprocessor, who's job it will be to fix line numbers and if necessary aggregate results from array items. I'll see if I can put together a pull request. I think it should be pretty easy.

Member

ilyavolodin commented Feb 12, 2015

Sorry, wasn't clear, array wouldn't be represented in the output at all... Preprocessor will return an array of strings that would be then sent to verify function (one at a time). Output of the verify function will be sent to postprocessor, who's job it will be to fix line numbers and if necessary aggregate results from array items. I'll see if I can put together a pull request. I think it should be pretty easy.

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 13, 2015

@ilyavolodin Looks like a really promising pull request, agree with @nzakas comments though.

My main concern is that a postprocessor should also know about the actual input data (to fix positional information), how would that work when decoupling post from pre this much?

wooorm commented Feb 13, 2015

@ilyavolodin Looks like a really promising pull request, agree with @nzakas comments though.

My main concern is that a postprocessor should also know about the actual input data (to fix positional information), how would that work when decoupling post from pre this much?

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 13, 2015

Member

@wooorm I guess postprocess should also take file name, but other then that, array of messages that will be passed into postprocess will match output of the preprocess, as in, if preprocess will return only one chunk, then postprocess will get an array of 1 as an input parameter. It will be up to the plugin to track relationship of files, chunks and offsets.

Member

ilyavolodin commented Feb 13, 2015

@wooorm I guess postprocess should also take file name, but other then that, array of messages that will be passed into postprocess will match output of the preprocess, as in, if preprocess will return only one chunk, then postprocess will get an array of 1 as an input parameter. It will be up to the plugin to track relationship of files, chunks and offsets.

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 13, 2015

@ilyavolodin That sounds great :)

wooorm commented Feb 13, 2015

@ilyavolodin That sounds great :)

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 15, 2015

Member

Moving discussion back to the issue per @nzakas comment.

You're concerned about using split() ?
We should really move this discussion back to the issue so that it's more available for everyone.

I'm not concerned about using split, however, it feels quite dirty to me to create an object structure and then parse part of that object manually. I would rather create an object in such a way that wouldn't require parsing, since we don't really have any limitations as to how it should look like.
In all honesty, it really feels like it preprocessors object should instead be an array of objects.

Member

ilyavolodin commented Feb 15, 2015

Moving discussion back to the issue per @nzakas comment.

You're concerned about using split() ?
We should really move this discussion back to the issue so that it's more available for everyone.

I'm not concerned about using split, however, it feels quite dirty to me to create an object structure and then parse part of that object manually. I would rather create an object in such a way that wouldn't require parsing, since we don't really have any limitations as to how it should look like.
In all honesty, it really feels like it preprocessors object should instead be an array of objects.

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 15, 2015

IMHO I like @nzakas proposal at #1832 (comment). Here’s how I envision it working with multiple extensions:

var processor = {
    preprocess: function() {},
    postprocess: function() {}
}

module.exports = {
    rules: {},
    rulesConfig: {},
    processors: {
        ".md": processor,
        ".markdown": processor
    }
};

wooorm commented Feb 15, 2015

IMHO I like @nzakas proposal at #1832 (comment). Here’s how I envision it working with multiple extensions:

var processor = {
    preprocess: function() {},
    postprocess: function() {}
}

module.exports = {
    rules: {},
    rulesConfig: {},
    processors: {
        ".md": processor,
        ".markdown": processor
    }
};
@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 15, 2015

Member

I guess that would work, but that means that any time a preprocessor will support multiple extensions you need to declare named object so that you can reuse it. I guess it's not that big of a deal though.

Member

ilyavolodin commented Feb 15, 2015

I guess that would work, but that means that any time a preprocessor will support multiple extensions you need to declare named object so that you can reuse it. I guess it's not that big of a deal though.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 16, 2015

Member

I like that suggestion. I'd imagine most people would not define entire plugins in a single file, similar to rules, so this makes a lot of sense to me.

Member

nzakas commented Feb 16, 2015

I like that suggestion. I'd imagine most people would not define entire plugins in a single file, similar to rules, so this makes a lot of sense to me.

@nzakas nzakas added accepted core enhancement and removed triage labels Feb 16, 2015

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 16, 2015

Member

Pull request updated.

Member

ilyavolodin commented Feb 16, 2015

Pull request updated.

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 25, 2015

@nzakas @ilyavolodin I just began work on this, and the major thing I’m still missing is some sort of “state”, which I can use to update the positional information (in the postprocess stage).

When I find the JavaScript embedded in Markdown, I’ve got access to the positional offset of said blocks, but when post processing the messages found by ESLint, that access is no longer available—there’s no input, only a filename.

A simple solution would be to have an object, collect, which is passed to both pre- and postprocess. That would allow plugin developer to store whatever they wanted, a cache, between corresponding processes.

What do you think?

P.S. why is there a filename passed to postprocess? And why not preprocess?

wooorm commented Feb 25, 2015

@nzakas @ilyavolodin I just began work on this, and the major thing I’m still missing is some sort of “state”, which I can use to update the positional information (in the postprocess stage).

When I find the JavaScript embedded in Markdown, I’ve got access to the positional offset of said blocks, but when post processing the messages found by ESLint, that access is no longer available—there’s no input, only a filename.

A simple solution would be to have an object, collect, which is passed to both pre- and postprocess. That would allow plugin developer to store whatever they wanted, a cache, between corresponding processes.

What do you think?

P.S. why is there a filename passed to postprocess? And why not preprocess?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 25, 2015

Member

@wooorm isn't state something you can collect during preprocess() and access during postprocess()? I'm not seeing a need for another method.

I think passing filename to only one is a bug (it's also not mentioned in the docs).

Keep in mind, ESLint runs synchronously, so preprocess() and postprocess() always run in predictable sequence.

Member

nzakas commented Feb 25, 2015

@wooorm isn't state something you can collect during preprocess() and access during postprocess()? I'm not seeing a need for another method.

I think passing filename to only one is a bug (it's also not mentioned in the docs).

Keep in mind, ESLint runs synchronously, so preprocess() and postprocess() always run in predictable sequence.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 25, 2015

Member

@nzakas is correct. Preprocess should build up a map of parts to offsets and then use that map in postprocess. There shouldn't be a reason to turn CLI into message bus and have it deliver information that it doesn't care about.
I'm not sure why do we need file name in preprocess. We need it in postprocess to modify error messages with the right file name, if preprocess returns bunch of parts (although, I'm not even sure that's necessary either).

Member

ilyavolodin commented Feb 25, 2015

@nzakas is correct. Preprocess should build up a map of parts to offsets and then use that map in postprocess. There shouldn't be a reason to turn CLI into message bus and have it deliver information that it doesn't care about.
I'm not sure why do we need file name in preprocess. We need it in postprocess to modify error messages with the right file name, if preprocess returns bunch of parts (although, I'm not even sure that's necessary either).

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 25, 2015

Member

@ilyavolodin I think it's nice continuity to have the filename passed to both. You could then quite easily create an object whose keys are the filenames and reference it in both preprocess() and postprocess().

Member

nzakas commented Feb 25, 2015

@ilyavolodin I think it's nice continuity to have the filename passed to both. You could then quite easily create an object whose keys are the filenames and reference it in both preprocess() and postprocess().

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 25, 2015

@nzakas (your first message) yes it is, and the global scope is a fine place, but IMHO it’s cleaner when having some sort of state, and not littering the global scope: but that’s personal I guess!

Re filename: I agree on adding it for both methods just for continuity!

wooorm commented Feb 25, 2015

@nzakas (your first message) yes it is, and the global scope is a fine place, but IMHO it’s cleaner when having some sort of state, and not littering the global scope: but that’s personal I guess!

Re filename: I agree on adding it for both methods just for continuity!

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Feb 26, 2015

Member

I just updated pull request with filename for preprocess.
@wooorm Node.js doesn't really have global scope (well, it does, but you wouldn't really be using it with global variable inside a module). You also have an option of creating an object with private variables and them just exposing two methods from it, preprocess and postprocess.

Member

ilyavolodin commented Feb 26, 2015

I just updated pull request with filename for preprocess.
@wooorm Node.js doesn't really have global scope (well, it does, but you wouldn't really be using it with global variable inside a module). You also have an option of creating an object with private variables and them just exposing two methods from it, preprocess and postprocess.

@wooorm

This comment has been minimized.

Show comment
Hide comment
@wooorm

wooorm Feb 27, 2015

@ilyavolodin @nzakas I got #1832 to work, and it works great!

wooorm commented Feb 27, 2015

@ilyavolodin @nzakas I got #1832 to work, and it works great!

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Feb 27, 2015

Member

Awesome!

Member

nzakas commented Feb 27, 2015

Awesome!

@nzakas nzakas closed this in bdac19c Feb 27, 2015

nzakas added a commit that referenced this issue Feb 27, 2015

Merge pull request #1832 from ilyavolodin/loaders
Update: Add support for custom preprocessors (fixes #1817)
@mik01aj

This comment has been minimized.

Show comment
Hide comment
@mik01aj

mik01aj Nov 12, 2015

Is there any official markdown plugin based on this API? I found only https://github.com/eslint/eslint-plugin-markdown and https://github.com/wooorm/eslint-md, but the first one is a plugin and the second one is a separate tool.

mik01aj commented Nov 12, 2015

Is there any official markdown plugin based on this API? I found only https://github.com/eslint/eslint-plugin-markdown and https://github.com/wooorm/eslint-md, but the first one is a plugin and the second one is a separate tool.

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.