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): [type-annotation-spacing] handle space between ? and : #3138

Merged
merged 4 commits into from Mar 28, 2021

Conversation

Knutas
Copy link
Contributor

@Knutas Knutas commented Mar 3, 2021

Fixes type-annotation-spacing rule to give an error if there is a space between ? and : in a type annotation.

Fixes #3140

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @Knutas!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #3138 (ae1372e) into master (9323eae) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3138      +/-   ##
==========================================
+ Coverage   92.88%   92.90%   +0.02%     
==========================================
  Files         315      316       +1     
  Lines       10706    10842     +136     
  Branches     3025     3065      +40     
==========================================
+ Hits         9944    10073     +129     
- Misses        342      343       +1     
- Partials      420      426       +6     
Flag Coverage Δ
unittest 92.90% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...eslint-plugin/src/rules/type-annotation-spacing.ts 80.82% <100.00%> (+0.82%) ⬆️
...-plugin/src/rules/no-unnecessary-type-assertion.ts 94.66% <0.00%> (-2.44%) ⬇️
packages/eslint-plugin/src/rules/unbound-method.ts 91.30% <0.00%> (-1.88%) ⬇️
.../eslint-plugin/src/rules/member-delimiter-style.ts 94.28% <0.00%> (-0.63%) ⬇️
...es/eslint-plugin/src/rules/no-floating-promises.ts 100.00% <0.00%> (ø)
...es/eslint-plugin/src/rules/object-curly-spacing.ts 100.00% <0.00%> (ø)
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts 100.00% <0.00%> (ø)
...ackages/eslint-plugin/src/util/getWrappingFixer.ts 93.93% <0.00%> (ø)
packages/typescript-estree/src/convert.ts 98.24% <0.00%> (+<0.01%) ⬆️
packages/eslint-plugin/src/util/types.ts 83.51% <0.00%> (+0.27%) ⬆️
... and 2 more

armano2
armano2 previously approved these changes Mar 3, 2021
@armano2 armano2 added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Mar 3, 2021
@armano2

This comment has been minimized.

@Knutas

This comment has been minimized.

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.

Thanks for the PR!

unless the before option is set to true.

before only applies to space before the pair.
I don't think overloading it with additional meaning is correct here.

If we wanted to allow space between the pair that should be a separate option - an option I would hold off on implementing for now (until someone asks for it).

@Knutas
Copy link
Contributor Author

Knutas commented Mar 3, 2021

So change it to always raise this error regardless of options?

@bradzacher
Copy link
Member

So change it to always raise this error regardless of options?

Yes please!

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Mar 3, 2021
@bradzacher bradzacher changed the title fix(eslint-plugin): [type-annotation-spacing] handle space after ? feat(eslint-plugin): [type-annotation-spacing] handle space after ? Mar 3, 2021
@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed bug Something isn't working labels Mar 3, 2021
@Knutas Knutas requested a review from bradzacher March 4, 2021 07:12
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 4, 2021
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.

logic itself LGTM, a few comments.
Thanks for your work on this!

@@ -177,6 +177,22 @@ export default util.createRule<Options, MessageIds>({
const { before, after } = getRules(ruleSet, typeAnnotation);

if (type === ':' && previousToken.value === '?') {
if (sourceCode.isSpaceBetween!(previousToken, punctuatorTokenStart)) {
Copy link
Member

Choose a reason for hiding this comment

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

isSpaceBetween was only added in ESLint v6.7.0
We currently support as far back as ESLint v5.0.0.

Meaning this is not safe.

You need to fall back to the old sourceCode.isSpaceBetweenTokens() API for those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that isSpaceBetween was already used by object-curly-spacing so I assumed it was fine to use.

if (sourceCode.isSpaceBetween!(previousToken, punctuatorTokenStart)) {
context.report({
node: punctuatorTokenStart,
messageId: 'unexpectedSpaceBefore',
Copy link
Member

Choose a reason for hiding this comment

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

I think we could add a custom message here for additional clarity.
Something like Unexpected space between the {{previousToken}} and the {{type}}.

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Mar 21, 2021
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 28, 2021
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 - thanks for your contribution!

@bradzacher bradzacher changed the title feat(eslint-plugin): [type-annotation-spacing] handle space after ? feat(eslint-plugin): [type-annotation-spacing] handle space between ? and : Mar 28, 2021
@bradzacher bradzacher merged commit 40bdb0b into typescript-eslint:master Mar 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[type-annotation-spacing] Space between ? and :
3 participants