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

Generic/JumbledIncrementer: fix inconsistency #443

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Apr 10, 2024

Description

The name of the sniff uses "Incrementer", while the error message uses "Incrementor". This feels inconsistent.

Based on some light research, both terms can be used interchangable, so I'm proposing to make the error message consistent with the sniff name, as changing the sniff name would be a breaking change.

Suggested changelog entry

Generic.CodeAnalysis.JumbledIncrementer: fix typo in warning message.

Related issues/external references

Seen while reviewing #385

Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

I'm not sure which is best, incrementors or incrementers, and I was not able to find either in the main dictionaries.

Regardless of that, I agree that the proposed change improves the consistency of the code. It is also worth mentioning that the original PMD project, which was used as a reference for this sniff, uses incrementer: https://docs.pmd-code.org/latest/pmd_rules_java_errorprone.html#jumbledincrementer.

I have two comments that I don't think need to be addressed in this PR necessarily, but are things that occurred to me while reviewing this PR that I wanted to share in case you think it is worth addressing them:

The sniff code uses the word incrementor one more time. Should it also be changed to incrementer?

  • This sniff also flags decrementers. Since we are already updating the error message, should it be changed to include decrementers as well?

The name of the sniff uses "Incrementer", while the error message uses "Incrementor". This feels inconsistent.

Based on some light research, both terms can be used interchangable, so I'm proposing to make the error message consistent with the sniff name, as changing the sniff name would be a breaking change.

Includes making the same change in an inline comment.
@jrfnl jrfnl force-pushed the feature/generic-jumbledincrementer-fix-typo branch from 7ec55a9 to e5569b6 Compare April 10, 2024 18:11
@jrfnl
Copy link
Member Author

jrfnl commented Apr 10, 2024

Thanks @DannyvdSluijs and @rodrigoprimo for looking this PR over.

The sniff code uses the word incrementor one more time. Should it also be changed to incrementer?

Good catch. Done.

This sniff also flags decrementers. Since we are already updating the error message, should it be changed to include decrementers as well?

I did consider this, but then, it may not even be an incrementer/decrementer:

$a = 'a';
for ($i = ''; strlen($i) < 10; $i . $a);

So to use the right word in all circumstances, I believe the logic in the sniff would need to be adjusted to take the operation being executed in the third expression into account. That's a much bigger change and something which I believe would be more appropriate to address in the context of #441.

Does that make sense ?

@rodrigoprimo
Copy link
Contributor

So to use the right word in all circumstances, I believe the logic in the sniff would need to be adjusted to take the operation being executed in the third expression into account. That's a much bigger change and something which I believe would be more appropriate to address in the context of #441.

Does that make sense ?

Sounds good to me. Thanks.

@jrfnl jrfnl merged commit dba1cc1 into master Apr 10, 2024
44 checks passed
@jrfnl jrfnl deleted the feature/generic-jumbledincrementer-fix-typo branch April 10, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants