search

fluid-project / fluid-lint-all

Consolidated linting logic free from any particular build technology
BSD 3-Clause "New" or "Revised" License
0 stars 5 forks source link

Fix array merging for includes and excludes (resolves #29). #33

Closed the-t-in-rtf closed 3 years ago

the-t-in-rtf commented 3 years ago

See #29 for details.

the-t-in-rtf commented 3 years ago

@greatislander, I cut 1.0.5-dev.20210416T143028Z.dfea15d.GH-29, which follows a strategy somewhere between what Antranig and I discussed here and what you asked for in #21:

The default excludes should be present in the merged configuration.

amb26 commented 3 years ago

I get the following test failures (13) on my system (Windows 7, node 14.15.4, npm 6.14.11)

13:09:52.736:  jq: FAIL: Module "Tests for the fluid-lint-all launcher." Test name "We should be able to exclude files included in a .gitignore file." - Message
: Check 'lintspaces.jsonindentation' should not have reported invalid content.
13:09:52.738:  jq: Source:     at pok (E:\source\gits\fluid-lint-all\node_modules\infusion\tests\test-core\jqUnit\js\jqUnit.js:109:15)
    at Object.fail (E:\source\gits\fluid-lint-all\node_modules\infusion\tests\test-core\jqUnit\js\jqUnit.js:126:13)
    at E:\source\gits\fluid-lint-all\tests\js\launcher-tests.js:36:24
    at ChildProcess.exithandler (child_process.js:315:5)
    at ChildProcess.emit (events.js:315:20)
    at maybeClose (internal/child_process.js:1048:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:288:5)
13:09:52.777:  jq: FAIL: Module "Tests for the fluid-lint-all launcher." Test name "We should be able to exclude files included in a .gitignore file." - Message
: Check 'lintspaces' should not have reported invalid content.
13:09:52.778:  jq: Source:     at pok (E:\source\gits\fluid-lint-all\node_modules\infusion\tests\test-core\jqUnit\js\jqUnit.js:109:15)
    at Object.fail (E:\source\gits\fluid-lint-all\node_modules\infusion\tests\test-core\jqUnit\js\jqUnit.js:126:13)
    at E:\source\gits\fluid-lint-all\tests\js\launcher-tests.js:36:24
    at ChildProcess.exithandler (child_process.js:315:5)
    at ChildProcess.emit (events.js:315:20)
    at maybeClose (internal/child_process.js:1048:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:288:5)

etc. And then also failure 

ERROR: Coverage for branches (88.6%) does not meet global threshold (90%)

These failures are also in main and presumably a result of merging https://github.com/fluid-project/fluid-lint-all/pull/19

I expect the cause of the failure is the parsing strategy at https://github.com/fluid-project/fluid-lint-all/pull/19/files#diff-17dc7e461493eba4d0b0b632a74d2ed2a64a0b4175208fedc110a3f2d8c2773bR50 which splits the file on "\n". Note that the recommended strategy for reading files by line on node is described in https://nodejs.org/api/readline.html

the-t-in-rtf commented 3 years ago

Note that the recommended strategy for reading files by line on node is described in https://nodejs.org/api/readline.html

There are several strategies on that page, all of which require converting to using asynchronous functions, which I would rather avoid if possible.

I updated the split to use a hopefully more robust regex, not sure if that helps. If that doesn't help, I would appreciate it if you could confirm with a debugger what the array of parsed excludes looks like when you run the tests.

the-t-in-rtf commented 3 years ago

I see the issues in a Windows 10 VM, but I also see that six entries are found. I think the problem might actually be with the pathing immediately below that spot.