-
Notifications
You must be signed in to change notification settings - Fork 202
Add a hint to avoid capitalisms #1608
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
base: master
Are you sure you want to change the base?
Conversation
src/Hint/NoCapitalisms.hs
Outdated
a:b:c:as -> [a,b,c] : trigrams (c:as) | ||
_otherwise -> [] | ||
|
||
--- these are copied from Hint.Naming --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this PR goes ahead, I would propose factoring out the code below.
src/Hint/NoCapitalisms.hs
Outdated
{- | ||
Detect uses of capitalisms | ||
|
||
Only allow up to two consecutive capital letters in identifiers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this exception? ID
? Personally I'd prefer uniformity here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be one case.
I'm all for uniformity, by I'm afraid not allowing this exception could make this rule very annoying. Let me do some statistics on our internal code base…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, prohibiting two consecutive capitals would catch an identifier like MVar
, I think that would be awkward.
I agree, we should favor the spelling Id
over ID
, but I can't think of a good rule here, that doesn't also catch, e.g., getIData
.
Maybe we could tighten the proposed rule to prohibit two consecutive capitals unless followed by a lower case letter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. There are many violations in the ecosystem that are utterly entrenched. Even if you try to be clever, for instance, allowing two capitals followed by lower-case / end of string, users will still need to do something about e.g. STM
, TBQueue
. There is no principled way to say the latter two are fine, but HTTP
is not.
Hence I think the ideal solution would be to trigger on all consecutive capitals (no special rules), and allow users to easily disable specific cases globally. Can warnings be parameterized in this way? The best I can think of is something like:
# .hlint.yaml
- ignore:
name: "No Capitalism"
within:
# I think this could work for functions.
- "Module.readTBQueue"
# Does this work for type / data declarations? Or does this assume
# module Another/TBQueue.hs?
- "Another.TBQueue"
This doesn't get you a global rule for ignoring e.g. IO
, but maybe it's workable enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this PR only complains about local definitions, not all occurrences of such names 🤔 Doesn't it? If it does, then definitions from other packages are not a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, though I am thinking of interop with upstream definitions, where re-using a captialized name is arguably more readable than adhering to this rule. For instance, I imagine quite a few people (most?) would say runInIO
is better than runInIo
, even for new definitions, because of how familiar IO
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this PR only complains about local definitions, not all occurrences of such names 🤔 Doesn't it? If it does, then definitions from other packages are not a problem.
This rule only checks top-level definitions of value, constructor, type and class identifiers; it does not check occurrences, be they imported or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# .hlint.yaml - ignore: name: "No Capitalism" within: # I think this could work for functions. - "Module.readTBQueue" # Does this work for type / data declarations? Or does this assume # module Another/TBQueue.hs? - "Another.TBQueue"This doesn't get you a global rule for ignoring e.g.
IO
, but maybe it's workable enough.
I don't understand the configuration language well enough to be able to confirm that your suggestion works. OTOH, ANN pragmas might be more effective for defining exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arybczak I have become convinced that the stricter rule is feasible, i.e., prohibiting all consecutive capitals, exceptions have to be allowed using pragmas.
</TEST> | ||
-} | ||
|
||
module Hint.NoCapitalisms(noCapitalismsHint) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"capitalism"? How about "NoCAPs"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalism, as in, an expression written in all caps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see such a meaning for capitalism in the dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, "NoCAPs" seems better since it doesn't have political connotations and shouldn't provoke unnecessary discussions, unlike the current name.
import GHC.Util | ||
|
||
noCapitalismsHint :: DeclHint | ||
noCapitalismsHint _ _ decl = [ remark Ignore "Avoid capitalisms" (reLoc (shorten decl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a Note to the hint, explaining what it does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the description at beginning of the file sufficient? I followed the style of Hint.Naming
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just a code comment. A Note is attached to a hint and is displayed with the hint. Alternatively, you can make the hint name more descriptive: "Avoid three consecutive capital letters" instead of "Avoid capitalisms".
src/Hint/NoCapitalisms.hs
Outdated
|
||
--- these are copied from Hint.Naming --- | ||
|
||
shorten :: LHsDecl GhcPs -> LHsDecl GhcPs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some comments explaining what this does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
I add a module
Hints.NoCapitalisms
that provides a hint to avoid capitalisms in identifiers, with some exceptions. E.g.are discouraged.
The hint has severity
Ignored
, so it is off by default.