lint: collect unused files #22

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

@@ -20,6 +20,7 @@ module.exports = clap.create('lint', '[fileOrPreset]')
 .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


 /**
 * @class File
 */

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


Not needed, see below

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

lib/common/files.js

@@ -296,6 +298,10 @@ var FileManager = function(baseURI, relBaseURI, console, flow){
 this.warns = [];

 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

@@ -91,7 +94,11 @@ function lint(config){
 l10nInfo: true
 }).concat([
 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

@@ -7,6 +7,9 @@ var extract = require('../extract');
 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 tyanas and lahmatiy 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: collect unused files #22

lint: collect unused files #22

smelukov commented Jun 15, 2017 •

edited

Loading

tyanas left a comment

tyanas Jun 16, 2017

lahmatiy Jun 20, 2017

tyanas Jun 16, 2017

smelukov commented Jun 16, 2017 •

edited

Loading

lahmatiy Jun 20, 2017

smelukov Jun 21, 2017

lahmatiy Jun 20, 2017

smelukov Jun 21, 2017

lahmatiy Jun 20, 2017

smelukov Jun 21, 2017

lahmatiy Jun 20, 2017

smelukov Jun 21, 2017

lahmatiy Jun 20, 2017

lahmatiy Jun 20, 2017

lahmatiy Jun 20, 2017

lahmatiy Jun 20, 2017

lahmatiy Jun 20, 2017

lahmatiy Jun 20, 2017

smelukov Jun 21, 2017 •

edited

Loading

lahmatiy Jun 28, 2017

lahmatiy Jun 28, 2017

smelukov Jun 29, 2017

lahmatiy Jun 28, 2017

lint: collect unused files #22

Are you sure you want to change the base?

lint: collect unused files #22

Conversation

smelukov commented Jun 15, 2017 • edited Loading

tyanas left a 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

smelukov commented Jun 16, 2017 • 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

smelukov Jun 21, 2017 • 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

smelukov commented Jun 15, 2017 •

edited

Loading

smelukov commented Jun 16, 2017 •

edited

Loading

smelukov Jun 21, 2017 •

edited

Loading