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): added new rule use-default-type-parameter #562

Conversation

JoshuaKGoldberg
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg commented May 27, 2019

Adds the equivalent of TSLint's use-default-type-parameter rule.

I'm not sure what to do about the lack of test coverage on the files, since most of them are for edge cases in the TypeScript API not finding information. Would you prefer nodes of unknown types be added as test cases? Will wait for further instructions...

@codecov
Copy link

codecov bot commented May 28, 2019

Codecov Report

Merging #562 into master will decrease coverage by 0.02%.
The diff coverage is 92.45%.

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
- Coverage   94.34%   94.31%   -0.03%     
==========================================
  Files         111      112       +1     
  Lines        4735     4787      +52     
  Branches     1315     1331      +16     
==========================================
+ Hits         4467     4515      +48     
- Misses        156      157       +1     
- Partials      112      115       +3
Impacted Files Coverage Δ
packages/eslint-plugin/src/util/misc.ts 88% <100%> (+3.78%) ⬆️
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️
...rc/rules/indent-new-do-not-use/BinarySearchTree.ts 100% <100%> (ø) ⬆️
...-plugin/src/rules/no-unnecessary-type-arguments.ts 91.11% <91.11%> (ø)
.../eslint-plugin/src/rules/no-useless-constructor.ts 100% <0%> (ø) ⬆️

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label May 28, 2019
bradzacher
bradzacher previously approved these changes Jun 24, 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.

LGTM

const arg = typeArguments[i];
const param = typeParameters[i];

// TODO: would like checker.areTypesEquivalent. https://github.com/Microsoft/TypeScript/issues/13502
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, sure. Would you like me to do this here or in a separate PR?

@bradzacher bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Jun 24, 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.

Thanks a lot for doing this, the implementation LGTM.

However, I can't help feeling this rule should be called no-unnecessary-type-arguments. I think that is subjectively more descriptive of what it enforces, and fits better with the existing conventions for these kinds of rules, both in our plugin and the ESLint ecosystem as a whole.

WDYT?

@JamesHenry JamesHenry added the awaiting response Issues waiting for a reply from the OP or another party label Jul 12, 2019
JamesHenry
JamesHenry previously approved these changes Jul 25, 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.

Thanks @JoshuaKGoldberg! Sorry about the conflict, hopefully it's easy enough to reconcile I've attempted to clean up the conflict via the GitHub UI

@JamesHenry JamesHenry removed 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready awaiting response Issues waiting for a reply from the OP or another party labels Jul 25, 2019
@JamesHenry JamesHenry merged commit 2b942ba into typescript-eslint:master Jul 25, 2019
@JoshuaKGoldberg JoshuaKGoldberg deleted the typescript-eslint-use-default-type-parameter branch July 25, 2019 01:17
@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
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants