add units suffix support to requireCamelCaseOrUpperCaseIdentifiers

[pabigot]

I've got projects where I want to use requireCamelCaseOrUpperCaseIdentifiers but have a higher priority requirement that variables and properties that hold measurements specify the units of measure in a suffix with an underscore separator. For example: ambientTemperature_cCel or centerFrequency_kHz.

My solution is to augment the configuration value for requireCamelCaseOrUpperCaseIdentifiers to take an optional units property which is an array of strings or regular expressions identifying allowed units that are permitted as a underscore-separated suffix. If an identifier has one of these suffixes and would pass the rule if the suffix was removed, then it is allowed with the suffix as well.

Is this enhancement of interest, or is there an existing way to solve this? (I noticed issue #1900 which is similar but I specifically want the non-suffix part to still be processed by the rule.)

[pabigot]

This could also be modified to use allowedSuffixes instead of units with the same behavior, and add allowedPrefixes for similar markings at the start of the identifier (e.g. opt_).

[pabigot]

On further consideration I've decided that the generalization to prefixes and suffixes makes sense, so have updated the pull request with a version that doesn't name the options in a way that limits them to units.

[markelog]

Coverage decreased (-0.01%) to 99.291%

?

[pabigot]

@markelog Resubmitted with comments addressed.

Coverage decrease was probably due to dc146d8b930b9d2be99d3efcf54beed631ef6557 using an overly verbose replacement of the leading/trailing underscore strip formerly applied only within a condition. I've refactored so it's back to a single line, but is still done separately and first on the argument that scope markers apply after prefix/suffix markers.

There was no change in unit tests for the prefix/suffix underscore stripping because it was intended to preserve existing behavior. I've added commit bd3b160046bf4bd01a311d97b605111d09dafb4f explicitly calls out an inconsistency in the unit tests. If the existing behavior is wrong, I'll rework; if the test description was wrong, hopefully coverage will be back up where it used to be.

[pabigot]

I see the issue with coverage and will fix it when I get a chance to figure out why the obvious test doesn't fail.

[pabigot]

@markelog current pull should be up to date and addresses all comments so far.

[pabigot]

@markelog Good comments; the refactoring is more clear and exposed gaps in the coverage tests.

I don't believe the travis failure on Node.js 4 is related to my changes.

[pabigot]

@markelog I'm going to need to implement allExcept as well, which will complete the functionality envisioned in #1900. OK to add that as a new commit on the branch underlying pr, or do you want it in a different pr?

[markelog]

Totally forgot about that ticket, can we introduce allExcept instead of adding two new options?

[pabigot]

We could. I didn't because there wasn't an obvious way to support prefixes and suffixes that still allowed the remainder of the identifier to be validated. E.g. the proposed opt_* would, naively, allow opt_this_thing where the semantics I want for a prefix would disallow that but permit opt_thisThing. Treating prefix and suffix explicitly also eliminates the need for start-of-match and end-of-match identifiers in regular expression exceptions.

So I think there's really three cases. They could be combined into one option:

"allExcept": {
   "match": [ "var_args", /^ignore/ ],
   "prefix": [ "opt" ],
   "suffix": [ "re", /[kMG]Hz/ ]
}

with:

  "allExcept": [ "var_args", /^ignore/ ]

accepted when only match exceptions are present.

[pabigot]

@markelog Looking at my last comment, there is a problem: I've been assuming I could use regular expressions, and that won't work with JSON configuration structures. I still think the three cases are worth supporting independently, but I'll need to rework the implementation to use glob minimatch, which does seem to have the expressibility I want.

I'll convert the implementation after we've resolved what the configuration should look like.

[pabigot]

@markelog I think this implementation needs significant rework which depends on agreeing on the specification and semantics, so I've updated #2029 with a proposal and request for input and am closing this PR. Thank you for your comments so far; I'll submit a new solution after we've worked out the details in the issue.