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 getter return rule #2543

Merged
merged 3 commits into from
Mar 1, 2024
Merged

Fix getter return rule #2543

merged 3 commits into from
Mar 1, 2024

Conversation

BlackSoulHub
Copy link
Contributor

No description provided.

@github-actions github-actions bot added A-linter Area - Linter A-ast Area - AST labels Feb 29, 2024
@Boshen
Copy link
Member

Boshen commented Feb 29, 2024

I think the better choice is to not check for TypeScript, to align with typescript-eslint

https://github.com/typescript-eslint/typescript-eslint/blob/6954a4a463f9a2a1c20e41c91ee2c75c953dcc9f/packages/eslint-plugin/src/configs/eslint-recommended-raw.ts#L24

Created a new issue: #2544

@BlackSoulHub
Copy link
Contributor Author

I think the better choice is to not check for TypeScript, to align with typescript-eslint

https://github.com/typescript-eslint/typescript-eslint/blob/6954a4a463f9a2a1c20e41c91ee2c75c953dcc9f/packages/eslint-plugin/src/configs/eslint-recommended-raw.ts#L24

Something like that?:

if ctx.source_type().is_typescript() {
    return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false);
}

@Boshen
Copy link
Member

Boshen commented Feb 29, 2024

I think the better choice is to not check for TypeScript, to align with typescript-eslint
https://github.com/typescript-eslint/typescript-eslint/blob/6954a4a463f9a2a1c20e41c91ee2c75c953dcc9f/packages/eslint-plugin/src/configs/eslint-recommended-raw.ts#L24

Something like that?:

if ctx.source_type().is_typescript() {
    return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false);
}

I'll do the whole batch from the list.

And since you're comfortable with code, you may try https://eslint.org/docs/latest/rules/no-unreachable which is the last unimplemented cfg rule.

There's also constructor-super, getter-return and no-fallthrough that are all in nursery, which need some real world testing so we can bring them to the correctness category.

@Boshen
Copy link
Member

Boshen commented Feb 29, 2024

Let's improve on this instead of skipping TypeScript files.

Copy link

codspeed-hq bot commented Feb 29, 2024

CodSpeed Performance Report

Merging #2543 will not alter performance

Comparing BlackSoulHub:main (8df4e9e) with main (dd31c64)

Summary

✅ 27 untouched benchmarks

@BlackSoulHub
Copy link
Contributor Author

I thought about what would happen to functions that return void? I check this later

@BlackSoulHub
Copy link
Contributor Author

And since you're comfortable with code, you may try https://eslint.org/docs/latest/rules/no-unreachable which is the last unimplemented cfg rule.

I'll try to do it. But what about this PR?

@Boshen
Copy link
Member

Boshen commented Mar 1, 2024

And since you're comfortable with code, you may try https://eslint.org/docs/latest/rules/no-unreachable which is the last unimplemented cfg rule.

I'll try to do it. But what about this PR?

I'll get this merged once all tests pass.

@Boshen Boshen enabled auto-merge (squash) March 1, 2024 13:39
@Boshen Boshen disabled auto-merge March 1, 2024 13:39
@Boshen Boshen enabled auto-merge (squash) March 1, 2024 13:39
@Boshen Boshen merged commit f00834d into oxc-project:main Mar 1, 2024
19 checks passed
a-rustacean pushed a commit to a-rustacean/oxc that referenced this pull request Mar 4, 2024
a-rustacean added a commit to a-rustacean/oxc that referenced this pull request Mar 4, 2024
a-rustacean added a commit to a-rustacean/oxc that referenced this pull request Mar 4, 2024
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants