IdentifierName: accept regex to exempt type name violations #4842
Conversation
Occasionally, type names need to follow conventions that violate the styleguide without being able to rely on inheritance or annotations for exemption. For example, JMX MBean and MXBean definitions will routinely violate the styleguide, and outside the JDK, `maven-failsafe-plugin` 3.5.2's default filter is a set of 3 violating patterns. Suppressing these violations quickly becomes tedious. Rather than Whac-A-Mole'ing tolerated violations, define a new option whose value is a regex such that, if a possibly-violating type name matches that regex, the type name is allowed anyway. References: google#4776 References: google#4832
| @@ -294,7 +299,9 @@ private static boolean isConformantLowerCamelName(String name) { | |||
| private boolean isConformantTypeName(String name) { | |||
| return !name.contains("_") | |||
| && isUpperCase(name.charAt(0)) | |||
| && (allowInitialismsInTypeName || !PROBABLE_INITIALISM.matcher(name).find()); | |||
| && (allowInitialismsInTypeName | |||
| || allowRegexInTypeName.matcher(name).matches() | |||
There was a problem hiding this comment.
AllowInitialismsInTypeName reads and works approximately like "allow initialisms [when they occur within a] type name". Similarly, AllowRegexInTypeName reads approximately like "allow [this] regex [when it matches within a] type name." However, I've used the anchored matches() instead of the unanchored find() and that makes me think that either
- the option name and this placement are incorrect, or
matches()is incorrect.
When I imagine how I would reason about this functionality, I arrive at "a simple-name that satisfies this pattern should be allowed, even when it would have been rejected". That mental model seems to interact with the fewest other rules and therefore be the simplest and easiest to reason about, but it can also be a blanket pass to ignore every other defined rule and that may be too much flexibility. In any case, to accurately implement that mental model
- the new regex should be the first thing we
matches(), and only on failure should we proceed to the original checks. - the regex's current fallback match-anything pattern would have to change, perhaps just into a presence/absence form.
- the name should probably not use the word "in", which can imply
find()-semantics, but be something more likeAllowTypeNameMatchingRegex.
A completely different approach is to parameterize PROBABLE_INITIALISM with ~its current value as the default. That grants maximal flexibility, but my objective with this proposal is not to bypass or challenge the styleguide's definition of camel case, only to provide what amounts to a high-level suppression mechanic. I think parameterizing PROBABLE_INITIALISM would be a fair bit more difficult to build and to use, and would grant an escape hatch that is too wide.
There was a problem hiding this comment.
For demonstration, a version that more faithfully matches my mental model: master...commonquail:error-prone:identifiername-allow-class-regex-harder. If I had to explain to somebody how this worked, that's the version I would rather explain.
| @@ -658,6 +658,21 @@ public void className_underscore() { | |||
| .doTest(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void className_badPattern_allowed() { | |||
There was a problem hiding this comment.
- Name?
- Explicit positive case?
- Interaction with other rules?
Here is an attempt at teaching
IdentifierNameto allow custom type name patterns that would otherwise be considered (true positive) violations, for use where external factors necessitate a violating form but the existingAllowInitialismsInTypeNameis either too accepting or fails to match.This implementation satisfies the JMX M(X)Bean type name pattern (closes #4776) and the Maven Failsafe plugin filter pattern (closes #4832), however, I consider some of its details contradictory; see notes.
For the new option I'm using the working name of
AllowRegexInTypeName, derived fromAllowInitialismsInTypeName, but I have reservations about that.The existence of
AllowInitialismsInTypeNameis not officially documented anywhere, much less its behavior, so neither is the new option I propose here. That seems to be the norm, too, but I'd be happy to change that.