Navigation Menu

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.

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

feat(eslint-plugin): [ban-types] Support namespaced type #616

Merged
merged 3 commits into from Jun 28, 2019
Merged

feat(eslint-plugin): [ban-types] Support namespaced type #616

merged 3 commits into from Jun 28, 2019

Conversation

golopot
Copy link
Contributor

@golopot golopot commented Jun 15, 2019

closes #135

This pr add support for banning namespaced type, with option like:

{
  types: {
    'React.FunctionComponent': {
      message: 'Use React.FC instead',
      fixWith: 'React.FC',
    },
  },
};

@codecov
Copy link

codecov bot commented Jun 15, 2019

Codecov Report

Merging #616 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   94.31%   94.37%   +0.06%     
==========================================
  Files         109      109              
  Lines        4536     4535       -1     
  Branches     1254     1252       -2     
==========================================
+ Hits         4278     4280       +2     
  Misses        148      148              
+ Partials      110      107       -3
Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/ban-types.ts 100% <100%> (+15.78%) ⬆️

@bradzacher bradzacher added the enhancement New feature or request label Jun 16, 2019
JamesHenry
JamesHenry previously approved these changes Jun 20, 2019
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Would only have subjective nits, looks good to me overall!

@JamesHenry JamesHenry added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Jun 20, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

The code you've got right now LGTM.

One thing that I would ask is could you please make it ignore the generic.
Unless I'm mistaken, if I do something like:

options = {
    'React.Component': { .... }
}

against this code:

class Foo extends React.Component<Props> {}

Then it will not detect it because getText will include the generic.
(If I'm wrong, then just add a test for that case, and we're good to go!)

@golopot
Copy link
Contributor Author

golopot commented Jun 28, 2019

Turned out it works fine with generics. I have added the tests. In short it works because TSTypeReference.typeName does not includes the type parameters parts.

@bradzacher bradzacher merged commit e325b72 into typescript-eslint:master Jun 28, 2019
@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.
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ban-types] Doesn't work with namespaced type
3 participants