Initial commit of Analyzer-based Linter. Ported over first linter 'unbalanced-delimiters' #1

usergenic · 2016-10-06T00:11:05Z

This change is

…balanced-delimiters'

rictic · 2016-10-06T17:17:33Z

src/linter.ts

+  public async lint(files: string[]): Promise<Warning[]> {
+    let warnings: Warning[] = [];
+    for (const file of files) {
+      const document: Document = await this.analyzer.analyze(file);


To catch stuff like syntax errors that stop us from creating a Document object, something like this is needed:

let document: Document; try { document = await this.analyzer.analyze(file); } catch (error) { if (error instanceof WarningCarryingException) { warnings.push(error.warning); continue; } throw error; }

Ok. I saw that in the linter demo code-- I'll yoink it for here.

Hopefully this is temporary?

What's the alternative? We've asked the analyzer to analyze a file, but it has a syntax error.

I guess we could return some sort of mostly empty Document object that has the parse error as a warning on it, but for most use cases I think that's a foot gun.

I have a catch now for the specific Error subclass WarningCarryingException as in the demo linter file in analyzer.

rictic · 2016-10-06T17:18:15Z

src/linter.ts

+
+  /**
+   * Given an array of filenames, lint the files and return an array of all
+   * warnings produced evaluating the linter rules.


Might be worth noting that the files need to be ones that the analyzer instance can resolve?

Double check that this change made it in.

rictic · 2016-10-06T17:19:51Z

src/rule.ts

+import {Warning} from 'polymer-analyzer/lib/warning/warning';
+
+// TODO(usergenic): Linter rules should have a standard interface for
+// configuring Warning severity and for setting their options.


I suspect we'll want to do warning severity filtering and modification in a central place, downstream of individual rules

That may be true. Rule configurability isn't an issue yet, just something I anticipate. Having a standard interface in constructor will be useful to configure Rule where they have configurability, like keyword blacklists or whatever. This also would allow polymer-linter.json or poylmer.json linter stanza to include per-rule config bits that are handed to the constructors in an obvious way.

Like, maybe instantiating the linter by way of a linter config would be similar to the way we do properties in Polymer. A rule class name on the left and in the typical case, the value on the right is just true or false and in the configurable case its an object i.e. { whitelist: ['h1','p','em','strong'] }

Anyways that's why I made them classes with a standard check method.

Definite +1 to making them configurable classes. I think we're going to want to add code and description properties to the Rule classes too.

rictic · 2016-10-06T17:25:51Z

src/rules/unbalanced-delimiters.ts

+    }
+
+    // Address binding expressions only for polymer elements.
+    for (const element of document.getByKind('polymer-element')) {


Would it be possible to structure this rule with a getDataBindingExpressions method? That'd be really broadly useful.

Hm, I guess not, as we're looking for stuff that's only almost legal data binding expressions.

It's a tricky one. If we extend analyzer to include binding expressions as features, it seems like it maybe shouldn't return binding expressions which are not properly formed, since that would be confusing. It totally depends on the way Analyzer treats returning features from scanners that are improperly formed or invalid in some way as a matter of convention.

Do you have any thoughts on whether Analyzer would report bad binding expressions as features?

Chatted out in person. I think consensus was to add this logic into the analyzer at some point, but it's fine to land it here for now.

rictic · 2016-10-06T17:26:44Z

src/rules/unbalanced-delimiters.ts

+               parsedHtml, element)) {
+        warnings.push(warning);
+      }
+    }


We should also check inside dom-bind templates.

Great point- I totally forgot dom-bind was a thing. Adding.

rictic · 2016-10-06T17:28:07Z

src/rules/unbalanced-delimiters.ts

+      return warnings;
+    }
+
+    const template = dom5.query(element.domModule, matchers.isTemplate);


An element's dom-module could be in another file. I'd recommend doing a getByKind('dom-module') instead.

Okay. Converting.

Since I also had to support dom-bind templates, I dropped the polymer element stuff and just search for <template> inside dom-modules and <template is="dom-bind"> without using the analyzed feature for now. Will migrate to feature when dom-bind templates are treated as such.

rictic · 2016-10-06T17:28:55Z

src/rules/unbalanced-delimiters.ts

+    const templateContent =
+        parse5.treeAdapters.default.getTemplateContent(template);
+
+    dom5.nodeWalkAll(templateContent, (node: parse5.ASTNode) => {


I think we want to recurse into inner template contents as well. (or maybe just dom-if and dom-repeat? but with that changing in 2.0, probably easier to just recurse into all template elements)

in theory one can vend other directive-style templates, though no one really does. So traversing into all templates is the easy and slightly more correct bet.

Recursing without discriminating. Added a comment to point out there's no condition applied and one may be considered in the future.

rictic · 2016-10-06T17:33:59Z

src/rules/unbalanced-delimiters.ts

+  }
+
+  private _extractBadBindingExpression(text: string): string|undefined {
+    // 4 cases, {{}, {}}, [[], []]


If there are just four cases, could we check for them individually? It would make for more legible code (and I say this as a generally regex-loving weirdo), and we could provide better warning messages. For me, I think the ideal message here would be:

Invalid polymer expression delimiters. You put '{{}' are you missing a closing }

I think it's worthwhile to say that they're polymer expression delimiters (and not like, a missing closing paren inside the polymer expression), and to say what we saw and how to fix.

I mostly copied and pasted this bit from polylint and hadn't really even thought of improving the matcher given there were 13 linters left to port over, but you make a good point about maintainability + legibility + much better result.

Step 1 complete: I made the messages nicer. I have not made bad binding expression match code nicer yet.

rictic · 2016-10-06T17:37:03Z

src/rules/unbalanced-delimiters.ts

+          code: 'unbalanced-delimiters',
+          message: `Expression ${attr
+                       .value
+                   } has unbalanced delimiters in attribute ${attr.name}`,


Generally I'd leave out information in the message that's expressed well in the sourceRange

That's a good rule to live by with these. Unredundantizing.

…ule now that its not needed.

justinfagnani

Man, I really have to get better about submitting my review comments!

justinfagnani · 2016-10-07T07:36:24Z

package.json

@@ -0,0 +1,44 @@
+{
+  "name": "polymer-linter",
+  "version": "1.0.0",


Let's start with a pre-release tag: 1.0.0-alpha.1

justinfagnani · 2016-10-07T07:36:56Z

src/linter.ts

+ * http://polymer.github.io/PATENTS.txt
+ */
+
+'use strict';


not necessary, tsc adds this for us.

Oh! Good to know. Removed from all the things.

justinfagnani · 2016-10-07T07:38:30Z

src/linter.ts

+  public async lint(files: string[]): Promise<Warning[]> {
+    let warnings: Warning[] = [];
+    for (const file of files) {
+      const document: Document = await this.analyzer.analyze(file);


Hopefully this is temporary?

justinfagnani · 2016-10-07T07:41:09Z

src/rules/unbalanced-delimiters.ts

+      console.assert(element instanceof PolymerElement);
+
+      for (const warning of this._warningsForPolymerElement(
+               parsedHtml, element)) {


really funky wrapping here, can you add a variable to hold the warnings before the loop?

Take it up with clang-format! But okay :)

justinfagnani · 2016-10-07T07:42:22Z

src/rules/unbalanced-delimiters.ts

+    const templateContent =
+        parse5.treeAdapters.default.getTemplateContent(template);
+
+    dom5.nodeWalkAll(templateContent, (node: parse5.ASTNode) => {


in theory one can vend other directive-style templates, though no one really does. So traversing into all templates is the easy and slightly more correct bet.

…uggested.

rictic · 2016-10-11T23:53:58Z

src/expressions.ts

+function primaryName(expression: string): string {
+  // TODO(usergenic): Remove this commented out section copied in from polylint
+  // if (expression.name) {
+  //   expression = expression.name;


Looks like this was sometimes an estree.Identifier, I say aggressively delete.

rictic · 2016-10-12T00:02:07Z

src/expressions.ts

+ * @param {string} type One of 'computed', 'literal', or 'reference'
+ * @param {string} raw  The unparsed expression
+ */
+export class ParsedExpression {


Recommend instead making this:

type ParsedExpression = Computed | Literal | Reference

And make each of those three types an interface with a string literal type field. That way we can separate out the fields and type safety will help us in a lot of ways.

This is how we're typing estree, and it's paid off hugely. It lets you switch over expression.type.. lots of stuff.

rictic · 2016-10-12T00:03:34Z

src/linter.ts

+
+  /**
+   * Given an array of filenames, lint the files and return an array of all
+   * warnings produced evaluating the linter rules.


Double check that this change made it in.

rictic · 2016-10-12T00:10:06Z

src/rules/native-attribute-binding.ts

+              continue;
+            }
+            warnings.push({
+              code: 'native-attribute-binding',


Here and elsewhere in this file, this isn't about native attribute binding so much as attributes which have no corresponding property.

Recommend updating the names, code, and message here.

Also, do we need to add the attr value and name here? With the new warning printer it will show you the line of code with full info.

rictic · 2016-10-12T00:10:58Z

src/rules/native-attribute-binding.ts

+    if (nativeAttributes.has(name)) {
+      return false;
+    }
+    if (name.indexOf('data-') === 0 && attr.name[name.length - 1] !== '$') {


Why are we checking whether attr.name ends with $ here? Shouldn't it be checked elsewhere already?

rictic · 2016-10-12T00:12:35Z

src/rules/native-attribute-binding.ts

+
+  private _isBindingExpression(expression: string): boolean {
+    const bindingMatch = expressionParser.extractBindingExpression(expression);
+    return !!bindingMatch || false;


AFAIK !!foo || false is equal to !!foo for all values of foo.

rictic · 2016-10-12T00:16:03Z

test/sample/my-element-collection.html

@@ -0,0 +1,17 @@
+<link rel="import" href="../../polymer/polymer.html">


Is this used in a later commit?

rictic · 2016-10-12T00:20:26Z

src/test/native-attribute-binding_test.ts

+          'The expression [[myVars]] bound to attribute \'class\' should use $= instead of =',
+      severity: Severity.ERROR,
+      sourceRange: {
+        end: {column: 28, line: 11},


Recommend using the WarningPrinter#printUnderlinedText() trick I used in my analyzer PR recently to test these source ranges. Makes them more maintainable and more easily reviewed.

rictic · 2016-10-12T00:21:54Z

test/sample/imports/bind-to-data.html

+subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt
+-->
+<dom-module id="bind-to-data">
+  <template>


Consider putting this into a dom-bind instead

rictic · 2016-10-12T00:22:49Z

test/sample/jsconf/README.md

@@ -0,0 +1,12 @@
+# JSConformance sample files


Wow, was this actually implemented in original polylint?

rictic · 2016-10-12T00:26:24Z

This is good work, but would it be possible to break it up into separate PRs? You're adding code at a faster rate than we can get it reviewed, which is a temporary situation, but it also means that nothing can land until we can catch up with your full speed run!

… yet. Extracted by rictic from usergenic's #1 PR

… yet (#10) * Initial scaffolding for the new linter, without any rules implemented yet. Extracted by rictic from usergenic's #1 PR * Tighten down and standardize tsconfig * Improve error handling. We should never crash based on an analysis error, instead we could just turn that error into a warning on the related file. * Standardize on tslint from analyzer, + the new prefer-const * Also recover from errors thrown from lint rules.

googlebot · 2016-12-22T00:33:28Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

rictic · 2016-12-22T00:34:29Z

(I resolved the conflicts with master through the github UI, but it looks like it created a merge commit. Might want to rebase that out.)

rictic · 2016-12-22T00:34:38Z

Also be quiet googlebot.

usergenic · 2017-01-11T19:48:35Z

closing this PR because @rictic's work obviates this bit-o scaffolding

Initial commit of Analyzer-based Linter. Ported over first linter 'un…

0bb82d2

…balanced-delimiters'

rictic suggested changes Oct 6, 2016

View reviewed changes

Upgraded to polymer-analyzer 2.0.0-alpha.12 and removed the utils mod…

2a5a238

…ule now that its not needed.

justinfagnani reviewed Oct 10, 2016

View reviewed changes

usergenic added 11 commits October 11, 2016 12:10

Set version to 1.0.0-alpha.1

95d6cda

Added support for WarningCarryingException handling in Linter.

87102ef

Added a Rule to catch WarningCarryingException.

1d9ae8b

Address PR comments for unbalanced delimiters rule.

8731da5

Bumped analyzer version to 2.0.0-alpha.14

7433baf

Added linter behavior/test to deal with WarningsCarryingException

d192602

Updated analyzer warnings test for style

7e32f93

Added native attribute binding rule.

e189edc

Added all the old polylint sample fixtures.

93a0c41

Updated README ever-so-slightly.

0a4211d

Updated the bad binding expression messages to be nice like @rictic s…

e45c72a

…uggested.

rictic suggested changes Oct 12, 2016

View reviewed changes

rictic pushed a commit that referenced this pull request Dec 21, 2016

Initial scaffolding for the new linter, without any rules implemented…

538a040

… yet. Extracted by rictic from usergenic's #1 PR

rictic mentioned this pull request Dec 21, 2016

Initial scaffolding for the new linter, without any rules implemented yet #10

Merged

Merge branch 'master' into brand-new-linter

38fea61

usergenic closed this Jan 11, 2017

rictic mentioned this pull request Feb 15, 2017

Add unbalanced polymer databinding delimitors rule. #23

Merged

1 task

rictic mentioned this pull request Jan 3, 2019

Get warnings from the new linter Polymer/tools#935

Closed

		@@ -0,0 +1,17 @@
		<link rel="import" href="../../polymer/polymer.html">

Initial commit of Analyzer-based Linter. Ported over first linter 'unbalanced-delimiters' #1

Initial commit of Analyzer-based Linter. Ported over first linter 'unbalanced-delimiters' #1

Conversation

usergenic commented Oct 6, 2016 • edited by rictic Loading

rictic Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rictic Oct 10, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

usergenic Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

rictic Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rictic Oct 6, 2016 • edited Loading

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

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usergenic Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usergenic Oct 11, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

rictic commented Oct 12, 2016

googlebot commented Dec 22, 2016

rictic commented Dec 22, 2016

rictic commented Dec 22, 2016

usergenic commented Jan 11, 2017

usergenic commented Oct 6, 2016 •

edited by rictic

Loading

rictic Oct 6, 2016 •

edited

Loading

rictic Oct 10, 2016 •

edited

Loading

usergenic Oct 11, 2016 •

edited

Loading

rictic Oct 6, 2016 •

edited

Loading

rictic Oct 6, 2016 •

edited

Loading

usergenic Oct 11, 2016 •

edited

Loading

usergenic Oct 11, 2016 •

edited

Loading