-
Notifications
You must be signed in to change notification settings - Fork 207
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?
Changes from 2 commits
dcce769
bd5cb3c
1a107f5
82c4457
c268f6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| {-# LANGUAGE FlexibleContexts #-} | ||
| {-# LANGUAGE LambdaCase #-} | ||
| {- | ||
| Detect uses of capitalisms | ||
|
|
||
| Only allow up to two consecutive capital letters in top level | ||
| identifiers. | ||
|
|
||
| Identifiers containing underscores are exempted from thus rule. | ||
| Identifiers of FFI bindings are exempted from thus rule. | ||
|
|
||
| Locally bound identifiers and module names are not checked. | ||
|
|
||
| <TEST> | ||
| data LHsDecl | ||
| class FOO a where -- @Ignore | ||
| class Foo a where getFOO :: Bool | ||
| data Foo = Bar | BAAZ -- @Ignore | ||
| data Foo = B_ar | BAAZ -- @Ignore | ||
| data Foo = Bar | B_AAZ | ||
| data OTPToken = OTPToken -- @Ignore | ||
| data OTP_Token = Foo | ||
| sendSMS = _ -- @Ignore | ||
| runTLS = _ -- @Ignore | ||
| runTLSSocket = _ -- @Ignore | ||
| runTLS_Socket | ||
| newtype TLSSettings = TLSSettings -- @Ignore | ||
| tlsSettings | ||
| data CertSettings = CertSettings | ||
| tlsServerHooks | ||
| tlsServerDHEParams = _ -- @Ignore | ||
| type WarpTLSException = () -- @Ignore | ||
| get_SMS | ||
| runCI | ||
| foreign import ccall _FIREMISSLES :: IO () | ||
| getSMS :: IO () -- @Ignore | ||
| gFOO = _ -- @Ignore | ||
| geFOO = _ -- @Ignore | ||
| getFOO = _ -- @Ignore | ||
| </TEST> | ||
| -} | ||
|
|
||
| module Hint.NoCapitalisms(noCapitalismsHint) where | ||
|
|
||
| import Hint.Type | ||
| import Data.List.Extra as E | ||
| import Data.List.NonEmpty as NE | ||
| import Data.Char | ||
| import Data.Maybe | ||
|
|
||
| import GHC.Types.Basic | ||
| import GHC.Types.SourceText | ||
| import GHC.Data.FastString | ||
| import GHC.Hs.Decls | ||
| import GHC.Hs.Extension | ||
| import GHC.Hs | ||
| import GHC.Types.SrcLoc | ||
|
|
||
| import Language.Haskell.GhclibParserEx.GHC.Hs.Decls | ||
| import Language.Haskell.GhclibParserEx.GHC.Utils.Outputable | ||
| import GHC.Util | ||
|
|
||
| noCapitalismsHint :: DeclHint | ||
| noCapitalismsHint _ _ decl = [ remark Ignore "Avoid capitalisms" (reLoc (shorten decl)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a Note to the hint, explaining what it does?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". |
||
| | not $ isForD decl | ||
| , name <- nubOrd $ getNames decl | ||
| , not $ hasUnderscore name | ||
| , hasCapitalism name | ||
| ] | ||
|
|
||
| hasUnderscore :: String -> Bool | ||
| hasUnderscore = elem '_' | ||
|
|
||
| hasCapitalism :: String -> Bool | ||
| hasCapitalism s = any isAllUpper (trigrams s) | ||
| where | ||
| isAllUpper = all isUpper | ||
|
|
||
| trigrams :: String -> [String] | ||
| trigrams = \case | ||
| a:b:c:as -> [a,b,c] : trigrams (b:c:as) | ||
| _otherwise -> [] | ||
|
|
||
| --- these are copied from Hint.Naming --- | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| shorten :: LHsDecl GhcPs -> LHsDecl GhcPs | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add some comments explaining what this does.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
| shorten (L locDecl (ValD ttg0 bind@(FunBind _ _ matchGroup@(MG FromSource (L locMatches matches))))) = | ||
| L locDecl (ValD ttg0 bind {fun_matches = matchGroup {mg_alts = L locMatches $ E.map shortenMatch matches}}) | ||
| shorten (L locDecl (ValD ttg0 bind@(PatBind _ _ _ grhss@(GRHSs _ rhss _)))) = | ||
| L locDecl (ValD ttg0 bind {pat_rhs = grhss {grhssGRHSs = E.map shortenLGRHS rhss}}) | ||
| shorten x = x | ||
|
|
||
| shortenMatch :: LMatch GhcPs (LHsExpr GhcPs) -> LMatch GhcPs (LHsExpr GhcPs) | ||
| shortenMatch (L locMatch match@(Match _ _ _ grhss@(GRHSs _ rhss _))) = | ||
| L locMatch match {m_grhss = grhss {grhssGRHSs = E.map shortenLGRHS rhss}} | ||
|
|
||
| shortenLGRHS :: LGRHS GhcPs (LHsExpr GhcPs) -> LGRHS GhcPs (LHsExpr GhcPs) | ||
| shortenLGRHS (L locGRHS (GRHS ttg0 guards (L locExpr _))) = | ||
| L locGRHS (GRHS ttg0 guards (L locExpr dots)) | ||
| where | ||
| dots :: HsExpr GhcPs | ||
| dots = HsLit noExtField (HsString (SourceText (fsLit "...")) (fsLit "...")) | ||
|
|
||
| getNames :: LHsDecl GhcPs -> [String] | ||
| getNames decl = maybeToList (declName decl) ++ getConstructorNames (unLoc decl) | ||
|
|
||
| getConstructorNames :: HsDecl GhcPs -> [String] | ||
| getConstructorNames tycld = case tycld of | ||
| (TyClD _ (DataDecl _ _ _ _ (HsDataDefn _ _ _ _ (NewTypeCon con) _))) -> conNames [con] | ||
| (TyClD _ (DataDecl _ _ _ _ (HsDataDefn _ _ _ _ (DataTypeCons _ cons) _))) -> conNames cons | ||
| _ -> [] | ||
| where | ||
| conNames :: [LConDecl GhcPs] -> [String] | ||
| conNames = concatMap (E.map unsafePrettyPrint . conNamesInDecl . unLoc) | ||
|
|
||
| conNamesInDecl :: ConDecl GhcPs -> [LIdP GhcPs] | ||
| conNamesInDecl ConDeclH98 {con_name = name} = [name] | ||
| conNamesInDecl ConDeclGADT {con_names = names} = NE.toList names | ||
|
|
||
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.