Skip to content

Conversation

uniqueiniquity
Copy link
Contributor

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

@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

j-f1
j-f1 previously requested changes Jan 30, 2019
Copy link
Contributor

@j-f1 j-f1 left a comment

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 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%> (ø)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants