Add acronym visibility controls - #2724
Conversation
|
I don't think we should enable the rule by default, having a mixture of acronym capitalizations in a single file isn't a great situation |
| /// Whether renaming this declaration cannot affect a public or serialized API contract. | ||
| func declarationCanBeRenamed( | ||
| _ declaration: Declaration, | ||
| upTo maximumVisibility: Visibility? |
There was a problem hiding this comment.
Should we have a single --rename-visbility option across all rules instead of one each?
There was a problem hiding this comment.
That would be problematic if we are planning to give them different defaults
There was a problem hiding this comment.
Potentially we could add a context-dependent "default" case to the enum, then each rule could interpret that as it chooses, but I think that might be more confusing than what we have now
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2724 +/- ##
===========================================
+ Coverage 95.56% 95.58% +0.02%
===========================================
Files 185 185
Lines 28810 28882 +72
===========================================
+ Hits 27532 27608 +76
+ Misses 1278 1274 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Should the options be made self documenting by being renamed to That also allows for possible future |
|
@rgoldberg how would min-visibility be applied? |
|
@nicklockwood I haven't looked through the implementation, but it would seem to me that keeping acronym naming consistent and/or avoiding typos is more important the more visible something is, because more people will look at public symbols than private symbols. I could understand limiting symbol renaming only at a public level because you want to ensure a public API is clean, but maybe you deal with third-party APIs that don't follow your naming conventions at internal or lower visibility, and you want to align your symbols with the third-party symbols without exposing them to the public. I thus wasn't sure which end the options limited until I checked their documentation. The primary purpose is self-documentation, but it also allows the easy adoption of min values in the future, too. Maybe in some circumstances min or max won't make sense, but standardizing across all options makes all |
|
@rgoldberg I'm inclined to leave it as-is for now, but it's not a big problem to rename it and deprecate the old name in future if the need arises |
3 participants
This PR extends the
acronymsrule with an--acronym-visibilityoption to control the visibility level of symbols to be renamed. This was inspired by the similar--typo-visibilityoption in thecommonTyposrule.@calda I defaulted this to
internalto match the existing behavior of the rule, but do you think it would be better to make itfileprivate, then the rule could safely be enabled by default?