Polymer lint directive scanner

[rictic]

For https://github.com/Polymer/polymer-linter/issues/58

[rictic]

Requirements:

• Supports comments in HTML or Javascript
• Tracks the source range of the comment
• Tracks what lint rule names are enabled and disabled by the comment

Proposed Syntax (🚲🏘ing welcome):

  <!-- polymer-lint enable: rule-name1 rule-name2 -->

  <!-- polymer-lint disable: rule-name2 rule-name3 -->

// polymer-lint enable: rule-name1 rule-name2

/* polymer-lint disable: rule-name1 rule-name2 */

[FredKSchott]

Some feedback/suggestions from thinking about this:

Comma separate rule list:

This lets us leave the door open for arguments/augmenting rules in the future.

<!-- polymer-lint enable: rule-name1, rule-name2 -->

hypotetical future support (total straw man):
<!-- polymer-lint enable: rule-name1 warn-only, rule-name2 -->

Support disabling of all rules for code blocks:

/* polymer-lint disable */

/* polymer-lint enable */

Support disabling for entire files:

If a disable comment is included at the top of a file, the entire file should inherit those disable rules.

[abdonrd]

You've probably seen it, but just in case... ESLint: Disabling Rules with Inline Comments

[FredKSchott]

I'm trying to think of how we can support this generally without building one-off polymer-linter support into the analyzer. This kind of detection will be useful for more than just polymer-linter.

What if we included comments as features in a document? Then we could build the ability to, before running a rule check on a document, convert any polymer-lint enable/polymer-lint disable comment pairs into source ranges. This would be enough information for a rule to know whether it should report an error or not. If we're worried about this being too noisy, we could make support for them optional / not have them be returned by the default getFeatures().

If we don't want to add comments as features at all, the only other way I could see this supported generally would be a way to walk astNodes for comments. Each language would need to encapsulate its own walker, probably within a document method.

@rictic @justinfagnani wdyt?

[FredKSchott]

@abdonrd Thanks for linking, I just read through that myself.

One thing I like that they do is have // comments affect only the line they're on, or the next line. But because we support languages that only have a single form for comments (like HTML) I don't think this is something we should tackle now. Or at the very least, we could force block comments (not support // polymer-lint ...) so that we could add support later.

[rictic]

IMO, adding a scanner for something isn't exactly building support for it into the analyzer. The goal is for scanners to be pluggable into an analyzer, and while there are still some missing pieces for this in the analyzer, the core of it is there and working.

It is very useful for this to be a scanner, as then we get all of the caching advantages of the analyzer.

[rictic]

Two nice things I see in the eslint syntax: the ability to disable/reenable linting for a range, and turning the ability to disable a lint rule only for the following line.

[FredKSchott]

@rictic Ah gotcha, I didn't know we'd added support for pluggable scanners. Any documentation available to play around with?

[rictic]

As I said, there's still missing pieces. Currently you have to tell the analyzer the scanners that you want it to use when it's created, but the idea has always been to move to something more dynamic. For now, baking in polymer-lint scanners seems no worse than baking in polymer mixin scanners. In the future they'll be broken out but for now it seems fine to hard code.

[FredKSchott]

@rictic that's useful context, thanks. Okay I feel better about a built-in lint derivative scanner if it's written in a way that can be pulled out later.

There may still be a way to write this for more flexible usage, maybe as a general "comment-defined source range". I'll play around a bit but stay focused on the polymer-linter usecase.

[MaKleSoft]

Funny, I built something very much like this for our internal linting tool not so long ago. It's even called DirectiveScanner! I remember having to use a private api to add the scanner and wishing for better support for custom scanners / general extensibility back then.

[justinfagnani]

@rictic and @FredKSchott I'd encourage exploring a few things in this design:

1. Some similarity, or ideally compatibility, with existing directive formats. At the least some survey of common directive formats in use now. Are there any that are more structured than /* name options */?
2. A slightly more general directive format than just linter directives. I can imagine them being useful in the bundler, for service worker and push manifest generation, compilation, etc.
3. At least documentation, but possible more structure, around attached vs detached comments, and as an extension, directives. Most comments don't make sense as standalone features because they're attached to some other structure. Currently we parse out jsdoc and attach that separately, so could we do the same with directives? Then, some comments are (maybe not-so) clearly meant to be unattached, some eslint directive apply to the whole file, or for every line following the comment. It'd be nice if the analyzer could determine whether a directive was attached or not at scan time.
4. Relatedly, I think we need to know what scope we need out of directives. I think there's maybe whole file, line range, attached, and line scopes. Do we need only one or all?

We don't need an exhaustive, detailed design right now, but it'd be nice to hit some of these points.

[FredKSchott]

@justinfagnani sounds good, I didn't originally think this was worth a design doc but if we are going to go the "more general directive scanner" route then I think that makes sense.

I'll create a new issue to discuss the solution/design I've been working on.

[dman777]

I could use this feature right now.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.