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

fix(eslint-plugin): [consistent-type-assertions] wrap object return value with parentheses #6885

Conversation

dora1998
Copy link
Contributor

@dora1998 dora1998 commented Apr 11, 2023

PR Checklist

Overview

Wrap type assertion with parentheses if object literal type and used as return value of arrow function.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @dora1998!

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.

@netlify
Copy link

netlify bot commented Apr 11, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 6948bfd
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/64ea4cca299c0700079a2ec9
😎 Deploy Preview https://deploy-preview-6885--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nx-cloud
Copy link

nx-cloud bot commented Apr 11, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6948bfd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 40 targets

Sent with 💌 from NxCloud.

@dora1998 dora1998 force-pushed the fix-consistent-type-assertion-fixer branch from bf78617 to d2b2a32 Compare April 11, 2023 15:57
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #6885 (6948bfd) into main (85f34da) will increase coverage by 0.00%.
The diff coverage is 85.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6885   +/-   ##
=======================================
  Coverage   87.22%   87.23%           
=======================================
  Files         386      387    +1     
  Lines       13383    13401   +18     
  Branches     3955     3962    +7     
=======================================
+ Hits        11673    11690   +17     
+ Misses       1328     1327    -1     
- Partials      382      384    +2     
Flag Coverage Δ
unittest 87.23% <85.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
...es/eslint-plugin/src/util/getOperatorPrecedence.ts 41.07% <ø> (+3.57%) ⬆️
...ackages/eslint-plugin/src/util/getWrappingFixer.ts 98.48% <75.00%> (-1.52%) ⬇️
...int-plugin/src/rules/consistent-type-assertions.ts 91.04% <85.71%> (-1.55%) ⬇️
packages/eslint-plugin/src/util/getWrappedCode.ts 100.00% <100.00%> (ø)

@dora1998 dora1998 force-pushed the fix-consistent-type-assertion-fixer branch from d2b2a32 to 8b43da5 Compare April 11, 2023 17:13
@dora1998 dora1998 changed the title fix(eslint-plugin): [consistent-type-assertions] wrap object type assertion with parentheses fix(eslint-plugin): [consistent-type-assertions] wrap object return value with parentheses Apr 11, 2023
@dora1998 dora1998 marked this pull request as ready for review April 11, 2023 17:56
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM in general - and love that you're fixing an issue you filed so quickly! 😍

Just requesting changes on generalizing the fix if possible - since it's very likely to come up again. Thanks!

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 11, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM, great test cases! Glad to see a reuse of the existing shared code. Thanks! 🙌

Since I haven't touched the getWrappingFixer util area much, marking as 1 approval and waiting for someone else to approve as well.

@JoshuaKGoldberg JoshuaKGoldberg added 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting response Issues waiting for a reply from the OP or another party labels Apr 13, 2023
@bradzacher bradzacher added the bug Something isn't working label Apr 14, 2023
@JoshuaKGoldberg
Copy link
Member

cc @armano2 - is this still something you want to look at?

If not, no worries, I feel comfortable merging :)

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.

big oof - the entire packages/eslint-plugin/src/util/getWrappingFixer.ts file probably needs to be rewritten because right now it uses its own hard-coded precedence logic, rather than using our precedence utilities

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labels Jul 18, 2023
@dora1998
Copy link
Contributor Author

dora1998 commented Jul 23, 2023

@bradzacher
I added getWrappedCode util function that wrap text in parens if needed because of precedence.
(I think it can also replace isLeftSideLowerPrecedence and isHigherPrecedenceThanUnary)

but It's hard to replace getWrapped with getWrappedCode because now we have to know precedence of node and all innerNodes..., so I'd like to hear your opinion.


By the way, lint and unit test is failing in CI but successful in my local environment. Will merging the main branch fix this...?

@@ -1,7 +1,10 @@
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES } from '@typescript-eslint/utils';
import { isBinaryExpression, isNewExpression } from 'tsutils';
Copy link
Member

Choose a reason for hiding this comment

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

Fun merge conflict artifact: we switched to ts-api-utils in v6.

@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 5, 2023
@JoshuaKGoldberg
Copy link
Member

By the way, lint and unit test is failing in CI but successful in my local environment. Will merging the main branch fix this...?

Yeah that should fix it. I don't like throwing extra work your way so I'll go ahead and merge now.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

@armano2 do you still have time for this?

No worries if not, now that you've taken at least one look I feel comfortable re-reviewing and approving & merging if it's good to go.

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Aug 15, 2023
@armano2 armano2 removed their request for review August 19, 2023 13:14
@dora1998
Copy link
Contributor Author

@JoshuaKGoldberg Sorry for my late reply...
Thank you so much for taking over my PR ✨

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Swell, thanks for such a thorough change! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit 23ac499 into typescript-eslint:main Aug 27, 2023
46 of 48 checks passed
@dora1998 dora1998 deleted the fix-consistent-type-assertion-fixer branch August 27, 2023 09:48
@alecmev
Copy link

alecmev commented Aug 30, 2023

Does this not make consistent-type-assertions typed?

ChrisW-B pushed a commit to ChrisW-B/PersonalApi that referenced this pull request Aug 30, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.4.1` -> `6.5.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/6.4.1/6.5.0) |
| [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.4.1` -> `6.5.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/6.4.1/6.5.0) |
| [@typescript-eslint/typescript-estree](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`6.4.1` -> `6.5.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2ftypescript-estree/6.4.1/6.5.0) |
| [netlify-cli](https://github.com/netlify/cli) | devDependencies | minor | [`16.1.0` -> `16.2.0`](https://renovatebot.com/diffs/npm/netlify-cli/16.1.0/16.2.0) |

---

### Release Notes

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v6.5.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#650-2023-08-28)

[Compare Source](typescript-eslint/typescript-eslint@v6.4.1...v6.5.0)

##### Bug Fixes

-   **eslint-plugin:** \[consistent-type-assertions] wrap object return value with parentheses ([#&#8203;6885](typescript-eslint/typescript-eslint#6885)) ([23ac499](typescript-eslint/typescript-eslint@23ac499))

You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

#### [6.4.1](typescript-eslint/typescript-eslint@v6.4.0...v6.4.1) (2023-08-21)

##### Bug Fixes

-   **eslint-plugin:** \[no-unnecessary-condition] false positives with branded types ([#&#8203;7466](typescript-eslint/typescript-eslint#7466)) ([b52658f](typescript-eslint/typescript-eslint@b52658f)), closes [#&#8203;7293](typescript-eslint/typescript-eslint#7293)

You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v6.5.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#650-2023-08-28)

[Compare Source](typescript-eslint/typescript-eslint@v6.4.1...v6.5.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

#### [6.4.1](typescript-eslint/typescript-eslint@v6.4.0...v6.4.1) (2023-08-21)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/typescript-estree)</summary>

### [`v6.5.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/typescript-estree/CHANGELOG.md#650-2023-08-28)

[Compare Source](typescript-eslint/typescript-eslint@v6.4.1...v6.5.0)

##### Features

-   bump supported TS version to 5.2 ([#&#8203;7535](typescript-eslint/typescript-eslint#7535)) ([f18c88d](typescript-eslint/typescript-eslint@f18c88d))
-   support Explicit Resource Management syntax for TS 5.2 ([#&#8203;7479](typescript-eslint/typescript-eslint#7479)) ([c11e05c](typescript-eslint/typescript-eslint@c11e05c))

You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

#### [6.4.1](typescript-eslint/typescript-eslint@v6.4.0...v6.4.1) (2023-08-21)

**Note:** Version bump only for package [@&#8203;typescript-eslint/typescript-estree](https://github.com/typescript-eslint/typescript-estree)

You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

</details>

<details>
<summary>netlify/cli (netlify-cli)</summary>

### [`v16.2.0`](https://github.com/netlify/cli/blob/HEAD/CHANGELOG.md#1620-2023-08-29)

[Compare Source](netlify/cli@v16.1.0...v16.2.0)

##### Features

-   add support for `context.params` in edge functions ([#&#8203;5964](netlify/cli#5964)) ([ed14e05](netlify/cli@ed14e05))
-   support custom function routes ([#&#8203;5954](netlify/cli#5954)) ([82b94b5](netlify/cli@82b94b5))

##### Bug Fixes

-   **deps:** update dependency [@&#8203;netlify/edge-bundler](https://github.com/netlify/edge-bundler) to v8.18.0 ([#&#8203;5953](netlify/cli#5953)) ([016cdf9](netlify/cli@016cdf9))
-   **deps:** update dependency [@&#8203;netlify/edge-bundler](https://github.com/netlify/edge-bundler) to v8.19.0 ([#&#8203;5971](netlify/cli#5971)) ([42478fd](netlify/cli@42478fd))
-   **deps:** update dependency [@&#8203;netlify/serverless-functions-api](https://github.com/netlify/serverless-functions-api) to v1.6.0 ([#&#8203;5935](netlify/cli#5935)) ([1d68354](netlify/cli@1d68354))
-   **deps:** update dependency [@&#8203;netlify/serverless-functions-api](https://github.com/netlify/serverless-functions-api) to v1.7.3 ([#&#8203;5957](netlify/cli#5957)) ([57d6627](netlify/cli@57d6627))
-   **deps:** update dependency lambda-local to v2.1.2 ([#&#8203;5967](netlify/cli#5967)) ([e592944](netlify/cli@e592944))
-   **deps:** update netlify packages ([#&#8203;5915](netlify/cli#5915)) ([1ad27ac](netlify/cli@1ad27ac))
-   **deps:** update netlify packages ([#&#8203;5965](netlify/cli#5965)) ([654c366](netlify/cli@654c366))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 3pm on Sunday" in timezone America/New_York, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi43NC4wIiwidXBkYXRlZEluVmVyIjoiMzYuNzQuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Reviewed-on: https://git.chriswb.dev/chrisw-b/PersonalApi/pulls/3
Reviewed-by: chrisw-b <chrisw-b@noreply.localhost>
Co-authored-by: Renovate Bot <renovate.bot@chrisb.xyz>
Co-committed-by: Renovate Bot <renovate.bot@chrisb.xyz>
@polyzen
Copy link

polyzen commented Aug 31, 2023

Does this not make consistent-type-assertions typed?

Seems to be the case. I'm using the following:

'plugin:@typescript-eslint/strict',
'plugin:@typescript-eslint/stylistic',

and getting the following error:
Error: Error while loading rule '@typescript-eslint/consistent-type-assertions': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Aug 31, 2023

I can't reproduce any issues using the rule in versions 6.5.0 of the parser and plugin. See https://github.com/JoshuaKGoldberg/repros/tree/consistent-type-assertions-type-information.

By the way, if you think there's a bug in typescript-eslint, the right way to ask is to file a new issue. The issue templates include helpful & necessary practices such as making sure you're on the latest version of all our packages. And it's much easier for us as maintainers to not lose track of things posted as issues.

Our contributing guide doesn't explicitly ask not to post tangential questions on closed PRs, so I filed an issue to add those docs. #7586


Just to emphasize: if you're experiencing an issue with typescript-eslint, please file an issue with the appropriate template. Please. 🙂

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: [consistent-type-assertions] autofix breaks type assertion of return value
6 participants