mgechev / codelyzer

Static analysis for Angular projects.
http://codelyzer.com/
MIT License
2.45k stars 233 forks source link

i18n rule with check-id and check-text does not handle the ng-container tag #530

Open sgallen opened 6 years ago

sgallen commented 6 years ago

Reference: PR #506

Today I updated to version 4.2.1 of Codelyzer, as I was finding the linter was incorrectly reporting errors with our use of select ICU expressions (see: https://angular.io/guide/i18n#translate-select). PR #506 seems to have addressed those issues (thanks!). Unfortunately, I'm now finding that when I use ng-container tags with i18n ids (see: https://angular.io/guide/i18n#translate-text-without-creating-an-element) I get linting errors, that I don't expect.

I've cloned the repo, locally adding 2 tests to the suite. They're identical tests except for the use of different tags (ng-container vs span). As you'll see further below ng-container fails while span works fine:

 66     it('NEW [ng-container]: should work with proper id', () => {
 67       const source = `
 68       @Component({
 69         template: \`
 70           <ng-container i18n="@@foo">Text</ng-container>
 71         \`
 72       })
 73       class Bar {}
 74       `;
 75       assertSuccess('i18n', source, ['check-id', 'check-text']);
 76     });
 77
 78     it('NEW [span]: should work with proper id', () => {
 79       const source = `
 80       @Component({
 81         template: \`
 82           <span i18n="@@foo">Text</span>
 83         \`
 84       })
 85       class Bar {}
 86       `;
 87       assertSuccess('i18n', source, ['check-id', 'check-text']);
 88     });

This is the output:

node@3bb72aa7d7e3:/codelyzer$ npm run test
npm info it worked if it ends with ok
npm info using npm@5.3.0
npm info using node@v8.5.0
npm info lifecycle codelyzer@4.2.1~pretest: codelyzer@4.2.1
npm info lifecycle codelyzer@4.2.1~test: codelyzer@4.2.1

> codelyzer@4.2.1 test /codelyzer
> rimraf dist && tsc && cp -r test/fixtures dist/test && mocha dist/test --recursive

  i18n
    check-id
      ✓ should work with proper id (50ms)
      ✓ should work with proper i18n attribute
      ✓ should work with proper id
      ✓ should work with proper id
      1) NEW [ng-container]: should work with proper id
      ✓ NEW [span]: should work with proper id
      ✓ should fail with missing id string
      ✓ should fail with missing id
      ✓ should fail with missing id

  8 passing (130ms)
  1 failing

  1) i18n check-id NEW [ng-container]: should work with proper id:
     AssertionError: expected false to be true
      at Function.assert.isTrue (node_modules/chai/lib/chai/interface/assert.js:332:31)
      at Object.assertSuccess (dist/test/testHelper.js:161:17)
      at Context.<anonymous> (dist/test/i18nRule.spec.js:30:26)

npm info lifecycle codelyzer@4.2.1~test: Failed to exec test script
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! codelyzer@4.2.1 test: `rimraf dist && tsc && cp -r test/fixtures dist/test && mocha dist/test --recursive`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the codelyzer@4.2.1 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/node/.npm/_logs/2018-03-04T17_39_02_030Z-debug.log

One caveat, is that I don't see any tests in which both check-id and check-text are set so it's possible I shouldn't be configuring the linter to use both. Not sure. This is the line that I have in my tslint file: "i18n": [true, "check-id", "check-text"]

mgechev commented 6 years ago

I believe @wKoza worked on this https://github.com/mgechev/codelyzer/pull/506/files

wKoza commented 6 years ago

Hi, That's what I feared . it is going to be difficult to distinguish ng-container declared by developer and generated by Angular.