Elm Review Rules on Mon, Feb 28th, 2022

elm-review-rules-adr.md

Elm Review Rules Architectural Design Record

Status

Proposed

Context

While Elm has an idiomatic way to do things regarding the architecture, there are various ways to use types and code. Much like JavaScript and Python have linting rules, Elm does as well to help enforce best practices that the team deems important.

Decision

My team have had multiple meetings to discuss the pro's/con's of each rule, and have decided on the following lists below. Inlcuded shows which ones we currently have enabled or want to. Not included are ones we reviewed but didn't want.

Included

• NoDuplicatePorts: as a code base grows, it's easy to hide the same names intentionally in a port module
• NoUnsafePorts: JavaScript cannot be trusted
• NoExposingEverything: This indicates a badly designed library/module.
• NoImportingEverything: excluding Html, this also indicates a bad design of your module.
• NoMissingTypeAnnotation: No type annotations results in a slower compiler and unclear compiler error messages. One of Elm's claims to fame is their compiler error messages, so this keeps that feature working.
• NoFloatIds: Edge case, but could easily be accidentally applied as a Float instead of an Int or String.
• NoRecursiveUpdate: Accidents happen; this prevents them... and can lead to race conditions, heh. Worth the cost we believe.
• NoInconsistentAliases: There is a bug if you mess up Html.style vs. Svg.style, and makes the code extremely unclear if you use various coding styles.
• NoModuleOnExposedNames: Either expose functions, or use the alias, not both.
• NoRedundantConcat: Result's in shorter, clearer, faster code.
• NoRegex: Elm encourages the use of the elm parser module. Also, Jesse hates Regular Expressions.
• NoSinglePatternCase: Using a case expression just to unpack a single type alias is ridicolous and verbose, better to destructure.
• NoTypeAliasConstructorCall: Using record constructor functions is dangerous and unclear; better to use record syntax.
• NoUnused.Dependencies: Keeps our elm.json clean.
• NoUnused.Exports: While Elm won't compile dead code, this can help in larger code bases to keep them clean or solve "why isn't my code being called?".
• NoUnused.Modules: same as above
• NoUnused.Patterns: Cleaner code
• NoUnused.Variables: Cleaner code
• NoPrimitiveTypeAlias: helps avoid primitive obession with compiler support

We need to figure these out; they require more investigations of costs and/or how to best configure.

• NoUnused.CustomTypeConstructorArgs: TODO: enable, but refactor some internal parsing first because of lots of holes _ _ _ _ _ _ _ (technical debt of not understanding how to parse a discriminated union from elm-graphql and had to use old Record syntax instead)
• NoUnused.CustomTypeConstructors: TODO: enable, just ensure configured for our phantom types
• NoUnused.Parameters: TODO: verify if holes negate this rule

Not Included

• NoDebug.Log: this is enforced by the elm compiler --optimize flag, but we turned off because we need to be able to debug before we go to production.
• NoDebug.TodoOrToString: Same as above
• NoUnusedPorts: While dead code is bad, you may still be figuring out how best to encode/decode in Elm. I don't think this should fail the build, but we could be convinced otherwise.
• NoEmptyText: While this sounded great at first, this really makes it hard to build a design from scratch when you're figuring things out. Html.nothing is better, yes, but CSS requires experiementation, and this rule gets in the way of that.
• NoMissingTypeAnnotationInLetIn: let is your escape hatch into imperative land, let's not punish people for writing hard to grok code in steps. Refactor later? Sure, but shouldn't fail the build.
• NoMissingTypeExpose: I'm not smart enough to know if this will affect phantom types, so have disabled for now.
• Docs.NoMissing: This is great for libraries, but we're not building those yet. This isn't applicable to applications.
• NoMissingSubscriptionsCall: This is for n00bs to Elm.
• NoAlways: We couldn't figure out why this was bad.
• Documentation-ReadmeLinksPointToCurrentVersion: We're not doing libraries yet.

Consequences

Enforcing these rules locally can be done via npm run review and the same command is run in the pipeline. If a rule becomes painful, or one is missing because of context (such as documentation for libraries), then we can re-access the rules. Thes are not set in stone. As we learn more about Elm, we reserve the right to change our minds.