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

Add no-identical-alternatives-in-ternary rule #2430

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

axetroy
Copy link
Contributor

@axetroy axetroy commented Aug 20, 2024

Close #2153

@axetroy axetroy marked this pull request as ready for review August 20, 2024 10:03
@sindresorhus
Copy link
Owner

I think the rule name should include ternary. So maybe no-identical-alternatives-in-ternary?

@@ -0,0 +1,22 @@
# Disallows nested ternary expressions with repeated alternatives
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
# Disallows nested ternary expressions with repeated alternatives
# Disallow nested ternary expressions with repeated alternatives


const MESSAGE_ID = 'no-identical-alternate';
const messages = {
[MESSAGE_ID]: 'Replace nested ternary expressions with a logical expression.',
Copy link
Owner

Choose a reason for hiding this comment

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

Should be more specific about what kind of ternary expression it targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about Replace repeated alternatives of ternary expressions with a logical expression. or Simplify repeated alternatives of ternary expressions. ?

@axetroy
Copy link
Contributor Author

axetroy commented Aug 20, 2024

I think the rule name should include ternary. So maybe no-identical-alternatives-in-ternary?

I'm not very good with rule naming, but I think this is a better name

@axetroy axetroy changed the title Add no-identical-alternate Add no-identical-alternatives-in-ternary Aug 20, 2024
@sindresorhus sindresorhus changed the title Add no-identical-alternatives-in-ternary Add no-identical-alternatives-in-ternary rule Aug 23, 2024
/** @param {import('estree').ConditionalExpression} node */
ConditionalExpression(node) {
// Check if the alternate is a ConditionalExpression
if (node.alternate && node.consequent.type === 'ConditionalExpression') {
Copy link
Collaborator

@fisker fisker Aug 23, 2024

Choose a reason for hiding this comment

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

I think we should go deeper,

a ? (b ? 1 : 2) : (c ? 1 : 3);
a ? (b ? 1 : 2) : (c ? 3 : 1)

should also be reported. I'm not sure about the equivalent... but there must be a better way to write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is too complicated, I don't think it can be done easily.

Copy link
Contributor Author

@axetroy axetroy Aug 25, 2024

Choose a reason for hiding this comment

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

I think we should ignore this situation, it is almost impossible to happen in the real world.

Or create another rule that names no-complex-ternary to disallow it.

@axetroy

This comment was marked as resolved.

@axetroy axetroy changed the title Add no-identical-alternatives-in-ternary rule [WIP] Add no-identical-alternatives-in-ternary rule Sep 10, 2024
@axetroy axetroy changed the title [WIP] Add no-identical-alternatives-in-ternary rule Add no-identical-alternatives-in-ternary rule Sep 10, 2024
@axetroy
Copy link
Contributor Author

axetroy commented Sep 10, 2024

I have reimplemented it. It is supports multiple lines and complex comments retention.

Now it's ready for review @fisker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: no-identical-alternate
3 participants