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.

Sign up for GitHub

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

Jump to bottom

fix: object-shorthand loses type parameters when auto-fixing #18438

Merged
merged 8 commits into from May 16, 2024
Merged

fix: object-shorthand loses type parameters when auto-fixing #18438

merged 8 commits into from May 16, 2024

Conversation

shulaoda
Copy link
Contributor

Fixes #18429

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fix object-shorthand loses type parameters when auto-fixing

Is there anything you'd like reviewers to focus on?

@shulaoda shulaoda requested a review from a team as a code owner May 10, 2024 16:06
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented May 10, 2024 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label May 10, 2024
@github-actions github-actions bot added the rule Relates to ESLint's core rules label May 10, 2024
@netlify Netlify
Copy link

netlify bot commented May 10, 2024 •
edited

✅ Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 8ff917f
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6643bfe32a4fea0008321055

@aladdin-add aladdin-add added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 11, 2024
aladdin-add
aladdin-add previously approved these changes May 11, 2024
View reviewed changes
Copy link
Member

@aladdin-add aladdin-add 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! Would like another review before merging.

@shulaoda shulaoda force-pushed the issue18429 branch 4 times, most recently from c0f90c8 to c4003f5 Compare May 11, 2024 12:02
@shulaoda shulaoda requested a review from aladdin-add May 11, 2024 12:12
@shulaoda
Copy link
Contributor Author

This way, wdyt? @aladdin-add

@mdjermanovic
Copy link
Member

Could we fix this by getting the range from the first token of node.value to the first token before => (including both tokens in the range)? If the arrow function is async, then async must be the first token (is that always the case?) so in that case we'd get the first token after the async token as the range start. A special case would be only a single-param function without parens of params, in which case we'd wrap it in ().

@shulaoda shulaoda force-pushed the issue18429 branch 3 times, most recently from 2d64687 to 9a68116 Compare May 12, 2024 13:18
mdjermanovic
mdjermanovic requested changes May 12, 2024
View reviewed changes
lib/rules/object-shorthand.js Outdated Show resolved Hide resolved
aladdin-add
aladdin-add previously approved these changes May 13, 2024
View reviewed changes
mdjermanovic
mdjermanovic approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic 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! Leaving open for @aladdin-add to re-review.

@shulaoda shulaoda requested a review from aladdin-add May 15, 2024 12:18
aladdin-add
aladdin-add approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@aladdin-add aladdin-add 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!

@aladdin-add aladdin-add merged commit 39fb0ee into eslint:main May 16, 2024
19 checks passed
@shulaoda shulaoda deleted the issue18429 branch May 16, 2024 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aladdin-add aladdin-add aladdin-add approved these changes

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly contributor pool rule Relates to ESLint's core rules
Projects
Triage
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: object-shorthand loses type parameters when auto-fixing
3 participants
@shulaoda @mdjermanovic @aladdin-add