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

Modify the diagnostic message of DontRepeatTypeInStaticProperties appropriately #918

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TTOzzi
Copy link
Contributor

@TTOzzi TTOzzi commented Jan 29, 2025

Resolve #854

DontRepeatTypeInStaticProperties checks whether the type is included in the static property's name using contains.
Since it triggers a diagnostic not only when the type appears as a suffix but also in other positions, I have appropriately modified the message.

Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

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

I don't think the message is the problem with this rule; the rule implementation itself is far too broad with what it detects. For example, consider one of the examples mentioned in that PR:

remove the suffix 'Data' from the name of the variable 'postgresDataType'

Presuming Data was the type of that property, that's an absolutely fine property name that doesn't violate this rule.

I don't remember precisely why we used contains in this implementation, unless it was a lackluster attempt at also catching plural terms. But at a minimum, I think we should be doing a suffix check instead of a contains check.

If you're really interested in exploring this, we could gain more insight by looking at how the compiler itself (in ClangImporter) stems type-name suffixes off the names of imported Objective-C names, and reimplement similar logic here to detect those cases.

@TTOzzi
Copy link
Contributor Author

TTOzzi commented Jan 29, 2025

I don't think the message is the problem with this rule; the rule implementation itself is far too broad with what it detects. For example, consider one of the examples mentioned in that PR:

remove the suffix 'Data' from the name of the variable 'postgresDataType'

Presuming Data was the type of that property, that's an absolutely fine property name that doesn't violate this rule.

I don't remember precisely why we used contains in this implementation, unless it was a lackluster attempt at also catching plural terms. But at a minimum, I think we should be doing a suffix check instead of a contains check.

If you're really interested in exploring this, we could gain more insight by looking at how the compiler itself (in ClangImporter) stems type-name suffixes off the names of imported Objective-C names, and reimplement similar logic here to detect those cases.

Oh, I see. Thank you for your kind advice.
I’ll refer to the ClangImporter logic and reimplement it accordingly!

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.

DontRepeatTypeInStaticProperties message doesn't match what it detect
2 participants