Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(trackBy): fix rule when ngFor statements include let assignments #483

Merged
merged 4 commits into from
Jan 9, 2018
Merged

fix(trackBy): fix rule when ngFor statements include let assignments #483

merged 4 commits into from
Jan 9, 2018

Conversation

alisd23
Copy link
Contributor

@alisd23 alisd23 commented Jan 9, 2018

  • No longer split by "=" as this broke *ngFor statements with let i = index statements or similar.
  • Add test to cover cases like this.
  • Fix line length lint error (maybe only on windows).

@alisd23
Copy link
Contributor Author

alisd23 commented Jan 9, 2018 •
edited

I also upgraded node-sass on my side because npm install would not work for me with that version as it couldn't find the binding (this is on Windows). I think it doesn't exist for modern versions of node, but I'm not 100% sure of the exact issue. Let me know if you want me to revert that upgrade, and I can just use the CI to see if the tests are passing.

mgechev
mgechev reviewed Jan 9, 2018
View reviewed changes
src/trackByFunctionRule.ts Outdated
@@ -24,28 +24,26 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

const ngForExpressionRe = new RegExp(/\*ngFor="(.+)"/);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/\*ngFor\s*=\s*'|"(.+)'|"/ this might be more accurate it will catch*ngFor = 'foobar' as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgechev Yep, good point. Will add that in. Also did you see my comment above about node-sass?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node-sass is fine, we use it only in the tests.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you also add a test case with different quotes and whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added a couple more. Should cover all the cases in the Regex.

@mgechev
Copy link
Owner

mgechev commented Jan 9, 2018

@alisd23 thanks for the PR! Left one comment regarding the regexp.

@mgechev mgechev merged commit 18b7ad9 into mgechev:master Jan 9, 2018
@alisd23 alisd23 deleted the fix-trackBy-with-let branch January 9, 2018 11:21
@mgechev mgechev mentioned this pull request Jan 9, 2018
Closed
mgechev added a commit that referenced this pull request Jan 16, 2018
@mgechev
Merge branch 'master' of github.com:mgechev/codelyzer
bd8386b 
* 'master' of github.com:mgechev/codelyzer:
  fix(i18n): do not warn with interpolation in i18n element (#485)
  fix(trackBy): fix rule when ngFor statements include let assignments (#483)
  fix(core): update the CSS parser (#482)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgechev mgechev mgechev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alisd23 @mgechev