Adapt to analyzer, build, linter v3 #948

valdrinkoshi · 2018-01-11T19:31:48Z

CHANGELOG.md has been updated

valdrinkoshi · 2018-01-11T19:32:28Z

src/lint/lint.ts

@@ -320,8 +320,9 @@ async function fix(
      await applyEdits(edits, makeParseLoader(analyzer, analysis));

  for (const [newPath, newContents] of editedFiles) {
+    const filePath = newPath.replace('file://', '').replace(config.root, '');


this is ugly, I'm probably missing something, but makes the --fix work

We need a common utility for this, but something like:

const url = new URL(newPath); let documentPath = decodeURIComponent(documentURL.pathname); if (isWindows() && documentPath.startsWith('/')) { documentPath = documentPath.substring(1); }

I moved this url resolution in util, method resolvePath. I'm using https://www.npmjs.com/package/url-js for getting the URL object, as it's a minimal package with no deps.

justinfagnani · 2018-02-23T00:54:29Z

@valdrinkoshi can you rebase and try to upgrade to the latest Analyzer 3 preview?

valdrinkoshi · 2018-02-23T20:23:21Z

@justinfagnani I've rebased and updated deps to latest analyzer and linter (forgot the linter previously!)

I'm still seeing some issues with path resolution: when invoking polymer lint I get warnings with the full path, and when invoking polymer lint --fix the warning doesn't get resolved:

$ echo "<dom-module name='foo-elem'></dom-module>" > test.html
$ polymer lint test.html -r polymer-2-hybrid


<dom-module name='foo-elem'></dom-module>
            ~~~~

file:///Users/valdrin/dev/polymer-cli/test.html(1,13) warning [dom-module-invalid-attrs] - Use the "id" attribute rather than "name" to associate the tagName of an element with its dom-module.

Found 1 warning.

$ polymer lint test.html -r polymer-2-hybrid --fix
No fixes to apply.

@rictic any ideas on this one? I haven't been following the progresses on polymer-linter lately

stramel · 2018-02-23T21:35:56Z

@valdrinkoshi That fix wasn't part of the lastest release. As of the latest release that should just warn and not be fixable. The next release will have a fix for that warning.

rictic · 2018-02-23T23:09:58Z

src/lint/lint.ts

@@ -320,8 +320,9 @@ async function fix(
      await applyEdits(edits, makeParseLoader(analyzer, analysis));

  for (const [newPath, newContents] of editedFiles) {
+    const documentPath = resolvePath(newPath);


I forgot! We added a method on FsUrlLoader to deal with this: FsUrlLoader#getFilePath()

That might mean that we'd have to pass down an FsUrlLoader into this function (I think this collection of functions has enough shared state that it should be a class, but that's a different issue)

oh nice! that makes the whole thing much simpler, we don't need to pass the config around but rather the urlLoader 👌 Fixed!

valdrinkoshi · 2018-02-24T02:16:41Z

@stramel good catch, I spent some time on that one w/o realizing i took a test from master instead of v3.0.0-pre.2 :x
Anyways, fixed and tested with:

$ echo "<dom-module id='foo-elem'><style></style><template></template></dom-module>" > test.html

$ polymer lint test.html -r polymer-2

<dom-module id='foo-elem'><style></style><template></template></dom-module>
                          ~~~~~~~

file:///Users/valdrin/dev/polymer-cli/test.html(1,27) warning [style-into-template] - Style tags should not be directchildren of <dom-module>, they should be moved into the <template>


Found 1 warning. 1 can be automatically fixed with --fix.

And

$ polymer lint test.html -r polymer-2 --fix
  Made 2 changes to file:///Users/valdrin/dev/polymer-cli/test.html

Fixed 1 warning.

The PR fails because many tests still expect relative paths printed (instead of absolute ones).
I think it would be nicer to have relative paths printed on console, no? @rictic WDYT? should we update tests to expect absolute paths here or fix this in the linter?

valdrinkoshi · 2018-02-26T18:55:30Z

src/lint/lint.ts

@@ -320,8 +320,12 @@ async function fix(
      await applyEdits(edits, makeParseLoader(analyzer, analysis));

  for (const [newPath, newContents] of editedFiles) {
-    await fs.writeFile(
-        path.join(config.root, newPath), newContents, {encoding: 'utf8'});
+    const documentPath = urlLoader.getFilePath(newPath);


newPath is already an absolute path, and urlLoader.getFilePath returns the same absolute path...I'm a bit confused now: either we use urlLoader to get the relative path here, or we don't use it at all and always print absolute paths. What should be done here?

Hm. newPath should be a URL, documentPath should be a FS path. Is that not what you're seeing?

stramel · 2018-03-06T20:27:18Z

src/lint/lint.ts

@@ -52,18 +51,19 @@ export async function lint(options: Options, config: ProjectConfig) {
    minimumSeverity: Severity.WARNING


@valdrinkoshi Do you mind linking up the new filesToIgnore option for the WarningFilter?

/cc @rictic This should cover the last bit of hook-up for ignoring files in linter I believe.

rictic

Lint looks good to me now. Just need to get build integration tests passing. Valdrin mentioned he's taking that on.

stramel · 2018-03-10T05:36:46Z

Can't wait for this so I can test upgrading my element's using all the new rules and fixes in linter! 🎉

stramel · 2018-03-10T08:13:02Z

package.json

@@ -15,7 +15,8 @@
    "test": "gulp test lint",
    "test:smoke": "gulp test",
    "test:integration": "gulp test:integration",
-    "format": "find src gulpfile.js \\( -iname '*.ts' -o -iname '*.js' \\) -not -path '*fixtures*' | xargs clang-format --style=file -i"
+    "format": "find src gulpfile.js \\( -iname '*.ts' -o -iname '*.js' \\) -not -path '*fixtures*' | xargs clang-format --style=file -i",


You can clean this command up a bit by doing,

"format": "find src gulpfile.js -iname '*.[jt]s' -not -path '*fixtures*' | xargs clang-format --style=file -i"

stramel · 2018-03-10T08:13:49Z

package.json

@@ -15,7 +15,8 @@
    "test": "gulp test lint",
    "test:smoke": "gulp test",
    "test:integration": "gulp test:integration",
-    "format": "find src gulpfile.js \\( -iname '*.ts' -o -iname '*.js' \\) -not -path '*fixtures*' | xargs clang-format --style=file -i"
+    "format": "find src gulpfile.js \\( -iname '*.ts' -o -iname '*.js' \\) -not -path '*fixtures*' | xargs clang-format --style=file -i",
+    "test:watch": "tsc-then -- mocha -c --ui tdd lib/test/integration/*_test.js lib/test/unit/*_test.js lib/test/unit/*/*_test.js"


You could shorten this line by using a more complex glob selector if you want,

"test:watch": "tsc-then -- mocha -c --ui tdd 'lib/test/@\\(integration\\|unit\\)/{,!\\(fixtures\\)/**/}/*_test.js'"

Or maybe just tsc-then -- gulp test test:integration since we already have a configured gulpfile?

The gulp command does too much, it compiles which we're using tsc-then to do quickly and incrementally.

Gulp also doesn't seem to do deduplication, so since both test and test:integration both depend on build it will end up compiling twice.

I often feel like I'm holding gulp wrong though, there's got to be a way to use it so that it doesn't do this… I would think.

I definitely thought gulp test test:integration would only run build once, maybe I'm wrong

rictic · 2018-03-12T18:58:17Z

Blocked by https://github.com/Polymer/polymer-build/issues/297

rictic · 2018-03-22T02:11:37Z

Linking in locally, it looks like Polymer/polymer-build#314 is all that's needed for the build integration tests to pass. Once Polymer/polymer-build#326 is merged and 3.0.0-pre.1 of build is released this may be ready to merge.

* If we can't get a file path for a document we need to change, fail and don't write any changes. * Improve the clarity of lint output. Specifically, counting the number of warnings fixed but not mentioning how many changes were made to each individual file, as the disparity is confusing. * Initialize PackageUrlResolver both package dir and component dir.

Also disable the test of custom component directory with build, as custom component directories seem like they only make sense with reusable elements, not applications, and applications are the only packages we expect to be built. Yell at me if this is a wrong assumption.

valdrinkoshi · 2018-03-22T21:37:36Z

src/lint/lint.ts

+  for (const [url, newContents] of editedFiles) {
+    const conversionResult = urlLoader.getFilePath(url);
+    if (conversionResult.successful === false) {
+      console.log(


should we use logger here and below?

+1 to this fatal error. The rest of the output shouldn't go through the logger I don't think, it should be logged directly to stdout as it's the primary purpose of running the command to log this stuff to stdout.

valdrinkoshi · 2018-03-22T21:38:29Z

package.json

+    "polymer-analyzer": "^3.0.0-pre.14",
+    "polymer-build": "^3.0.0-pre.1",
+    "polymer-linter": "^3.0.0-pre.3",
+    "polymer-project-config": "^3.9.0",


probably a merge issue, should keep 3.10.0 here

Dismissing review, updating polymer-build

valdrinkoshi · 2018-03-22T22:33:18Z

LGTM

valdrinkoshi commented Jan 11, 2018

View reviewed changes

valdrinkoshi requested a review from rictic January 11, 2018 19:35

valdrinkoshi force-pushed the adapt-to-analyzer-3 branch from cf93757 to 12f8bf3 Compare February 23, 2018 20:08

valdrinkoshi changed the title ~~Adapt to analyzer 3~~ Adapt to analyzer and linter v3 Feb 23, 2018

rictic reviewed Feb 23, 2018

View reviewed changes

valdrinkoshi commented Feb 26, 2018

View reviewed changes

stramel reviewed Mar 6, 2018

View reviewed changes

rictic mentioned this pull request Mar 7, 2018

Events on PR are not sorted by time PolymerLabs/project-health#259

Closed

rictic previously requested changes Mar 10, 2018

View reviewed changes

stramel reviewed Mar 10, 2018

View reviewed changes

valdrinkoshi changed the title ~~Adapt to analyzer and linter v3~~ Adapt to analyzer, build, linter v3 Mar 12, 2018

valdrinkoshi and others added 9 commits March 22, 2018 13:52

use analyzer and linter v3. Add resolvePath util.

1dc7ebf

leverage urlLoader

fa746cc

format filepath in logs

cdb3ade

update to latest analyzer & linter

8517efd

Update to polymer-build v3

26fba0b

Add build-library string type cast.

e0040bf

Update spelling of FsUrlLoader

e1653df

rictic force-pushed the adapt-to-analyzer-3 branch from 7432690 to e1653df Compare March 22, 2018 21:02

rictic added 2 commits March 22, 2018 14:21

Drop unused deps.

b937e88

Fix hybrid-incompatible code in 1.0 element template.

6a26e56

valdrinkoshi commented Mar 22, 2018

View reviewed changes

rictic added 2 commits March 22, 2018 14:40

Update polymer-project-config constraint.

b0bf5fb

Add a method to utils for getting a configured analyzer.

c32c77a

Increase lint test timeout.

4f9b7ce

rictic approved these changes Mar 22, 2018

View reviewed changes

rictic merged commit fef450a into master Mar 22, 2018

rictic deleted the adapt-to-analyzer-3 branch March 22, 2018 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt to analyzer, build, linter v3 #948

Adapt to analyzer, build, linter v3 #948

valdrinkoshi commented Jan 11, 2018

valdrinkoshi Jan 11, 2018

justinfagnani Feb 23, 2018

valdrinkoshi Feb 23, 2018

justinfagnani commented Feb 23, 2018

valdrinkoshi commented Feb 23, 2018

stramel commented Feb 23, 2018 •

edited

Loading

rictic Feb 23, 2018

valdrinkoshi Feb 24, 2018

valdrinkoshi commented Feb 24, 2018 •

edited

Loading

valdrinkoshi Feb 26, 2018

rictic Feb 26, 2018

stramel Mar 6, 2018 •

edited

Loading

rictic left a comment

stramel commented Mar 10, 2018

stramel Mar 10, 2018

stramel Mar 10, 2018 •

edited

Loading

rictic Mar 12, 2018

stramel Mar 16, 2018

rictic commented Mar 12, 2018

rictic commented Mar 22, 2018

valdrinkoshi Mar 22, 2018

rictic Mar 22, 2018

valdrinkoshi Mar 22, 2018

rictic Mar 22, 2018

valdrinkoshi commented Mar 22, 2018

		@@ -52,18 +51,19 @@ export async function lint(options: Options, config: ProjectConfig) {
		minimumSeverity: Severity.WARNING

Adapt to analyzer, build, linter v3 #948

Adapt to analyzer, build, linter v3 #948

Conversation

valdrinkoshi commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani commented Feb 23, 2018

valdrinkoshi commented Feb 23, 2018

stramel commented Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valdrinkoshi commented Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stramel Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

rictic left a comment

Choose a reason for hiding this comment

stramel commented Mar 10, 2018

Choose a reason for hiding this comment

stramel Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rictic commented Mar 12, 2018

rictic commented Mar 22, 2018

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

valdrinkoshi commented Mar 22, 2018

stramel commented Feb 23, 2018 •

edited

Loading

valdrinkoshi commented Feb 24, 2018 •

edited

Loading

stramel Mar 6, 2018 •

edited

Loading

stramel Mar 10, 2018 •

edited

Loading