10000 feat(eslint-plugin): add new rule no-for-in-array by uniqueiniquity · Pull Request #155 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

feat(eslint-plugin): add new rule no-for-in-array #155

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
Merged
merged 8 commits into from
Feb 7, 2019
Merged

feat(eslint-plugin): add new rule no-for-in-array #155

merged 8 commits into from
Feb 7, 2019

Conversation

uniqueiniquity
Copy link
Contributor

Adding equivalent of TSLint's no-for-in-array.

armano2
armano2 reviewed Jan 28, 2019
View reviewed changes
@uniqueiniquity
Copy link
Contributor Author

@armano2 How would I test line 38 in the rule? It's not a case that would ever really get hit if typescript-estree is behaving correctly. Should I just not worry about it?

@armano2
Copy link
Collaborator
armano2 commented Jan 28, 2019

hmm, in most cases nodes are present, and i think you should remove this if.

we have cases of nodes not from TS, and "broken" parts of code like
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/typescript-estree/src/convert.ts#L1290

armano2
armano2 suggested changes Jan 28, 2019
View reviewed changes
armano2
armano2 reviewed Jan 28, 2019
View reviewed changes
bradzacher
bradzacher reviewed Jan 30, 2019
View reviewed changes
j-f1
j-f1 previously requested changes Jan 30, 2019
View reviewed changes
Copy link
Contributor
@j-f1 j-f1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for contributing! A few small suggestions/questions:

@armano2 armano2 mentioned this pull request Feb 3, 2019
Merged
@armano2 armano2 added the enhancement: new plugin rule New rule request for eslint-plugin label Feb 3, 2019
@JamesHenry
Copy link
Member

@uniqueiniquity I think the confusion over the .md formatting has been resolved now, things should be easier once this is brought up to date with master

Thanks!

@JamesHenry
Copy link
Member

@j-f1 @armano2 Have your concerns been addressed? Please can you update your reviews?

@codecov
Copy link
codecov bot commented Feb 7, 2019

Codecov Report

Merging #155 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
+ Coverage   96.75%   96.76%   +0.01%     
==========================================
  Files          52       53       +1     
  Lines        2462     2472      +10     
  Branches      370      370              
==========================================
+ Hits         2382     2392      +10     
  Misses         40       40              
  Partials       40       40
Impacted Files Coverage Δ
packages/eslint-plugin/lib/util.js 97.22% <ø> (ø) ⬆️
...ackages/eslint-plugin/lib/rules/no-for-in-array.js 100% <100%> (ø)

@JamesHenry JamesHenry dismissed j-f1’s stale review February 7, 2019 01:47

Comments addressed

@JamesHenry JamesHenry merged commit 84162ba into typescript-eslint:master Feb 7, 2019
@bradzacher bradzacher mentioned this pull request Feb 7, 2019
Merged
kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this pull request Aug 27, 2019
@armano2 @JamesHenry
Migrate typescript-eslint-parser to 21.0.2 (typescript-eslint#174)
a58846c 
fixes: typescript-eslint#152, typescript-eslint#155, typescript-eslint#126, typescript-eslint#164, typescript-eslint#166, typescript-eslint#171, typescript-eslint#193, typescript-eslint#189, typescript-eslint#143, typescript-eslint#33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@j-f1 j-f1 j-f1 left review comments

@armano2 armano2 armano2 approved these changes

@JamesHenry JamesHenry JamesHenry approved these changes

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@uniqueiniquity @armano2 @JamesHenry @bradzacher @j-f1
0