lint: collect unused files by smelukov · Pull Request #22 · basisjs/basisjs-tools-build

smelukov · 2017-06-15T22:41:56Z

> basis lint --warn-unused-files src --reporter console

  ...

  unused files
    * /src/agent/app/page/field-demo/index.l10n
    * /src/agent/app/page/field-demo/index.js
    * /src/acti/ui/field/demo/template/demo.tmpl
    * /src/acti/ui/field/demo/template/demo-object.tmpl
    * /src/acti/ui/field/demo/template/demo-block.tmpl
    * /src/acti/ui/field/demo/template/demo-block.css
    * /src/acti/ui/field/demo/index.l10n
    * /src/acti/ui/field/demo/index.js

Merge after basisjs/basisjs#163 otherwise the linter will show the wrong result for <b:style src="./some.css"/>

tyanas

some tests would be great!

tyanas · 2017-06-16T10:10:46Z

lib/lint/reporter/process-warns.js

+var isChildProcess = typeof process.send == 'function'; // child process has send method
+var collectFiles = require('./collectFiles');
+
+module.exports = function(flow, options){


The interface was changed.
Have you considered adding [js]docs? Like what kind of objects should be flow and options now.

This module should be untouched (see below).

tyanas · 2017-06-16T10:16:46Z

lib/lint/index.js

+          }
+
+          cursor.linkTo.forEach(function(link){
+            // prevent to handling files that already handled


prevent to handling files that are already handled

smelukov · 2017-06-16T15:31:40Z

Now we supports:
dict.token('prop1').token('prop2').token('prop3')
prop1.prop2.prop3 will be marked as used.

And

<b:define name="someState" type="enum" values="prop1 prop2"/>

<div>
  {l10n:enum.{someState}}
</div>

enum.prop1 and enum.prop2 from template dictionary will be marked as used

lahmatiy · 2017-06-20T23:14:20Z

lib/lint/command.js

  .option('--no-color', 'Suppress color output')
  .option('--silent', 'No any output')

+  .option('--warn-unused-files <base>', 'Warn about unused files in specified path. Do not use with --js-cut-dev. It might cause incorrect result')


.option('--warn-unused-files <dir>', 'Warn about unused files for specified path. Avoid using with --js-cut-dev since it might cause to incorrect results')

lahmatiy · 2017-06-20T23:25:12Z

lib/common/files.js

 */

 function File(manager, cfg){
+  this.uniqueId = lastFileId++;


Not needed, see below

lahmatiy · 2017-06-20T23:25:30Z

lib/common/files.js

  this.readInfo = [];
+
+  // helpers
+  this.unixpath = unixpath;


Redundant, should be removed

lahmatiy · 2017-06-20T23:26:06Z

lib/lint/collectUsed/fileCollector.js

+  while (cursor = stack.pop())
+  {
+    // mark file as handled
+    handled[cursor.uniqueId] = true;


Use [Weak]Map, Luke!

I thought about Map, but it seems slow
But ok, I'll use it

lahmatiy · 2017-06-20T23:28:02Z

lib/lint/index.js

    function(flow){
-      flow.result = require('./reporter/process-warns')(flow.warns, options.filter);
+      if (options.warnUnusedFiles)
+        collectUsed.files(flow);


Bad pattern: function has a hidden side effect. As I realised it adds usedFiles to flow – it's confusing and should be explicit.

lahmatiy · 2017-06-20T23:33:11Z

lib/lint/reporter/process-warns.js

+var isChildProcess = typeof process.send == 'function'; // child process has send method
+var collectFiles = require('./collectFiles');
+
+module.exports = function(flow, options){


This module should be untouched (see below).

lahmatiy · 2017-06-20T23:41:01Z

lib/lint/collectUsed/files.js

+    }
+  });
+
+  flow.usedFiles = {


Should returns something rather than assign to flow.

lahmatiy · 2017-06-20T23:48:34Z

lib/lint/collectUsed/files.js

+  return file.filename && (basePath + file.filename).indexOf(collectPath + '/') === 0;
+}
+
+module.exports = function collectUsedFiles(flow){


There is much simplier way to find used files for some path:

var usedFiles = flow.files.queue .filter(function(file) { return file.fsFilename.indexOf(flow.options.warnUnusedFiles + '/') === 0; }) .map(...);

lahmatiy · 2017-06-20T23:49:51Z

lib/lint/index.js

+      if (options.warnUnusedFiles)
+        collectUsed.files(flow);
+    },
+    function(flow){


No need for two functions here.

lahmatiy · 2017-06-20T23:50:54Z

lib/lint/index.js

 var command = require('./command');
 var chalk = require('chalk');
 var isChildProcess = typeof process.send == 'function'; // child process has send method
+var collectUsed = {


No win to use an object here

l10n will be here soon :)

- decoupled flow and collectors - clear reporters

lahmatiy · 2017-06-28T23:39:05Z

lib/lint/reporter/parallel-process-unused-files.js

+    }
+
+    // warn about unused files
+    for (var unusedFile in unusedFiles)


I believe this loop doesn't needed and unusedFiles map is redundant

lahmatiy · 2017-06-28T23:42:15Z

lib/lint/unused/files.js

+    var collectedFiles = collectFiles(basePath);
+
+    usedFilesInfo.items.forEach(function(filename){
+      usedFiles[usedFilesInfo.basePath + filename] = true;


Why not delete collectedFiles[usedFile] here? usedFiles is redundant

cause it will affect flow.usedFiles
seems like it's not good

lahmatiy · 2017-06-28T23:43:03Z

lib/lint/unused/files.js

+      delete collectedFiles[usedFile];
+
+    for (var unusedName in collectedFiles) {
+      unusedName = unusedName.slice(process.cwd().length);


require('path').relative(...) ?

smelukov added the enhancement label Jun 15, 2017

tyanas requested review from lahmatiy and tyanas June 16, 2017 09:45

tyanas reviewed Jun 16, 2017

View reviewed changes

smelukov force-pushed the unused-files-linter branch 2 times, most recently from 291afb4 to b241c96 Compare June 16, 2017 12:05

smelukov changed the title ~~lint: unused file linter~~ lint: unused files and l10n tokens Jun 16, 2017

smelukov force-pushed the unused-files-linter branch 4 times, most recently from e902dad to f1528ff Compare June 16, 2017 15:07

Sergey Melyukov added 4 commits June 20, 2017 14:39

common/files: unique number id for files

8f274b0

lint: added --warn-unused-files parameter

010cf6f

lint: added files collector

83602ea

lint/reporter: collect and output unused files

8460b42

smelukov force-pushed the unused-files-linter branch from a8e1778 to 8460b42 Compare June 20, 2017 11:46

smelukov changed the title ~~lint: unused files and l10n tokens~~ lint: collect unused files Jun 20, 2017

lahmatiy reviewed Jun 20, 2017

View reviewed changes

Sergey Melyukov added 2 commits June 21, 2017 12:57

pr comments:

02a4c9a

- decoupled flow and collectors - clear reporters

lint: moved parellel unused files reporter to separate module

07ed3a8

smelukov self-assigned this Jun 22, 2017

lahmatiy reviewed Jun 28, 2017

View reviewed changes

Sergey Melyukov added 2 commits June 29, 2017 11:25

Merge remote-tracking branch 'origin/master' into unused-files-linter

5c70bc6

lint: minor improvements to unused-files reporter

20de275

Comments

Conversation

smelukov commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tyanas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smelukov commented Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smelukov Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smelukov commented Jun 15, 2017 •

edited

Loading

smelukov commented Jun 16, 2017 •

edited

Loading

smelukov Jun 21, 2017 •

edited

Loading