freckle / guides

Freckle guides and best practices

Freckle Guides

Front Row guides and best practices

Fancy Terms

Why do we use terms like "Monoid" and what do they mean?

Naming Conventions

Conventions for naming database entities, JSON serializations, etc.

Haskell

Best Practices

Best practices in a Haskell code base.

API

Best practices in our Haskell API specifically.

Style

Haskell style. TL;DR: use Fourmolu.

Testing

Haskell testing practices, work in progress.

Shell

Style

Shell style. TL;DR: use ShellCheck and shfmt.

Open Source

Processes and practices for our open source libraries.

guides's Issues

Link vs Location

In Status Codes, should "202 for async jobs (with Link header)" read "202 for async jobs (with Location header)"?

Compositional pipelines

I'd like to get a guide in about the following variations currently present in our code-bases:

A

f . g $ h x

B

f . g . h $ x

C

f $ g $ h x

D

f $ g $ h $ x

I prefer C; I believe @jdreaver prefers B. Can others express an opinion so we can see where we stand? If you'd just prefer no guide, feel free to say that.

Blank line between extension and module

(A)

{-# LANGUAGE OverloadedStrings #-}

module Foo
  ( foo
  ) where

(B)

{-# LANGUAGE OverloadedStrings #-}
module Foo
  ( foo
  ) where

(A')

{-# LANGUAGE OverloadedStrings #-}

-- |
--
-- Foo does the foo-ing
--
module Foo
  ( foo
  ) where

(B')

{-# LANGUAGE OverloadedStrings #-}
-- |
--
-- Foo does the foo-ing
--
module Foo
  ( foo
  ) where

Mention MS API Guidelines

https://github.com/microsoft/api-guidelines is a published guidelines we would like to adopt.

Where we have our own guides we mostly align already, and it also provides guidance on some things we don't yet do. Rather than re-invent any wheels, it'd be great to follow something established.

We should mention it in this repository and remove any guides that are conflicting or redundant with it.

Use prettier-markdown?

prettier-markdown was recently added as a default Restyler, and since we're running default configuration in this repository, it fired on the next PR to open.

Here are the changes it made:

• It enforces 80 columns

  We prefer and consistently do this in prose, so the only thing this affected were link references that violated:

  Before
  
  [link]: http://example.com/looooooooooooooooooooooooooooooooooooooooooooong
  
  After
  
  [link]:
    http://example.com/looooooooooooooooooooooooooooooooooooooooooooong

• It prefers _ over * for italics

  Before
  
  Hi *there*
  
  After
  
  Hi _there_

• It prefers - over * for unordered lists

  Before
  
  * Something
    * Sub-thing
  * Something else
  
  After
  
  - Something
    - Sub-thing
  - Something else

This is certainly not all it reformats, but it's the only things we were currently violating in the Haskell Style file, so I believe they are the salient points to discuss.

Generally, we would need to decide if we want to

1. accept what the tool does in the interest of automation,
2. configure the tool to match our preferences, or
3. disable the tool.

In this case, prettier doesn't believe in configuration, so (aside from very few overall options) we can't do the second option; this is an "in or out" decision.

What do we think? Feel free to react 👍 for option (1) or 👎 for option (3).

Personally, I'm in (surprise). These choices are purely asthetic, with a very minor technical pro for prettier in that it's less overloading of *, which may be objectively better. The "incorrect" forms are super ingrained in my fingers, which I would have to unlearn, which means I would want to make this our general Markdown style and enable prettier-markdown on all repositories.

Handlers Naming Conventions

I would like to propose a new naming convention for Yesod Handler modules.

Context

We do not have a well-established convention. We have a slight majority for:

FrontRow.Api.{Namespace}.Handler.{Resource}

Which is also what I see most often suggested for new modules.

I think there are intentional nuances to this convention that allow .{Resource} to be omitted/reversed (ActivityFeed.Handler) and/or {Namespace} to be split across .Handler. (Ela.AdaptiveSkill.Handler.Courses.Sessions), but these are not uniformly followed because I think such nuances (if they exist) are too complicated. We also don't consistently pluralize or singularize Namespace or Resource, with a slight preference for singular.

(For those interested, and who have not yet seen it, I have a private gist that catalogs our current modules. Please ask in Slack.)

A different but related inconsistency is how we name routes and route-constructors in general. I'm proposing something for that as part of this. The route-constructor should related the module back to the route, so standardizing the two together is important.

Motivation

All of this ends up making it difficult to (a) know where to put a new Handler and (b) locate an existing Handler.

An additional, but minor, issue is that having the "category" (Handler) in the middle of the path does not play well with any tooling that likes to operate in the style of Handler/* or */Handler.hs. Rarely do they support */Handler/*, especially when what comes before and after is itself inconsistent (split-Namespace) or often omitted (dropping Resource).

Proposal

route

{namespace}/{resource} {Namespace}{Resource} GET

module

FrontRow.Api.Handlers.{Namespace}.{Resource}

Optionality:

Please indicate which you prefer:

• A: Namespace and Resource are always singular (Handlers.Teacher.Student)
• B: Namespace and Resource are always plural (Handlers.Teachers.Students)

The rest of this proposal assumes (A), but I have no personal preference.

Notes:

• Each / in the {namespace} route is a . in the Namespace module
• If you end up with a Namespace like TeachersTeacher because of a route like /teachers/:id, which would lead to a module like Teacher.Teacher, omit the Teachers prefix -- see Examples, it's not as complicated as it sounds

Examples

teachers TeachersP:
  / TeachersR GET
  /#TeacherId TeacherP:
    / TeacherR GET
    /schools TeacherSchoolsR GET
    /students TeacherStudentsR GET

FrontRow.Api.Handlers.Teacher
  getTeachersR
  getTeacherR

FrontRow.Api.Handlers.Teacher.School
  getTeacherSchoolsR

FrontRow.Api.Handlers.Teacher.Student
  getTeacherStudentsR

Optional

We could define FrontRow.Api.Handlers which re-exports Handlers/*. We could maintain this manually, or write a small executable like hspec-discover that does it automatically. Such a project may even already exist.

Formalize PATCH Semantics for Sub-Resources

Formalize PATCH Semantics for Sub-Resources

Our Haskell API guide does a great job of laying out the intended behavior for PATCH endpoints.

However, the guidelines do not explicitly mention what the semantics should be when a sub-resource is involved, particularly for an empty patch.

For example, the patchSchoolR endpoint expects a SchoolPATCH body that can contains (nested) patch data for the RenaissanceSchoolClientPATCH sub-resource.

What is the expected behavior when the body is {} or {renaissanceSchoolClient: {}}, particularly when it comes to the updated_at fields. What if the sub-resource is not found?

Please see this comment for further context.

Add StrictData to default-extensions

I propose adding StrictData to our set of default-extensions.

In my experience, my PRs almost always receive the comment to add it (or !s) in any "data-type heavy modules". I'm not sure the distinction is worth it and would rather this be treated as a "sane default".

I don't totally understand the scenarios where it's not desired, but I believe they are rare enough that it's OK to expect a knowledgeable developer know to use the NoStrictData pragma in such modules. And a better trade-off for those that don't care to understand things deeply to get the StrictData behavior without thinking about it.

WDYT @cdparks @eborden @5outh ?

Brittany

We've toyed with the idea of Brittany and discussed it many times. Team buy-in seems a bit mixed: some are interested but skeptical, others are 💯 on board, and still others are merely tolerant. But I don't think anyone is against it.

To this point, we've held off on pushing Brittany because there are some things it doesn't handle and other things it handles poorly. I think it's time we revisit that idea.

This Issue is an attempt to reach consensus on:

1. Should we start auto-formatting Haskell modules with Brittany?
2. If so, should we do big-bang or some form of incremental?
3. If big-bang, when/how do we execute on that?

In addition to the discussion which I'm sure will ensue, I've also left some emoji in the body, which you can react to this Issue with if you hold those feelings.

Why Now?

Brittany, as of v0.11, has comment pragmas for ignoring declarations. There are some caveats (e.g. you can only ignore a top-level expression), but this means we can leave some expressions that Brittany makes objectively worse un-formatted. This is really the key reason we could make the jump now, if we wanted to.

1- Should we?

Please 👍 / 👎 this Issue.

2- Big-bang or incremental?

Big-bang

Brittany up all the Haskell in one monster commit. Maybe do a package at time?

PROS: sets an epoch in git history, avoids additional per-module discussions, Frontend had success doing this approach with prettier.

CONS: we won't get to do pragma-disabling if there are things we don't like, this means there is a higher burden of up-front research for how brittany will handle various code-constructs.

🎉 if in favor of this option.

Incremental

• New modules get Brittanied
• Existing modules get Brittanied when touched

PROS: we can see how Brittany changes things as it happens and make configuration tweaks or employ pragma-disabling.

CONS: we will have to evaluate how Brittany changes things as it happens, and likely have a lot of additional discussion on the specific points and if/when to pragma-disable. We wouldn't be able to turn on editor "auto-format-on-save" and/or maybe we'd have to be a bit more careful about separating re-formatting commits in PRs.

😄 if in favor of this option.

NOTE: nothing is preventing us from doing incremental for some short acclimation period then big-bang after.

3- When / How?

#22 (comment)

Appendix: Line Length

I'm going to strongly advocate for a Brittany setting of 80 columns, not 120.

I understand why we have our current guide of 120. I'm on board with that reasoning. We have long identifiers, we should allow longer lines because of that. I get it.

The difference is that with the current guide, while you're allowed 120 columns, you aren't required to use 120 columns. A human can still wrap lines more tightly when it makes sense and you don't end up with a module that makes no use of vertical space at all.

Brittany can't do that. If you configure Brittany with 120 columns, it will use that full width all the time. This drastically increases the number of lines that will actually reach closer to 120 and makes terrible use of vertical space. This is a case where the auto-formatter would be actively hurting code readability.

I'm only mentioning this now to avoid later surprise. If this Issue is gaining traction, but there is uncertainty about going with a setting of 80, I will attempt to make a stronger case through concrete examples where I think Brittany-120 would be harmful.

Toss me a ❤️ if you're in favor of 80.

Update response body Guides for API Types

We have been experimenting with a convention known as "API Types", which supersedes the old {Resource}{Method} naming for response body data types we currently describe. I think we're at a place where we know that's the way to go and we should update here.

default-extensions rule

The current rule is we do not use default-extensions; extensions should always be specified in the module that uses them.

We should revisit:

1. I believe people didn't realize this was coming in when it was introduced
2. Some cases such as NoImplicitPrelude really should be in defaults

This issue serves as additional awareness, to resolve (1) even if we change nothing.

To address (2), I would propose we change the guide to specify an explicit list of extensions that are allowed in default-extensions and enforce that all others are enabled by pragma. I would begin concervatively with [NoImplicitPrelude] being the safe-list, but welcome suggestions from others.

Haddock paragraph whitespace

I'm going to add some Haddock guidelines to this repo. AFAIK, what I would add is not controversial and I expect the PR to go by with minimal comments, except for one very specific thing: the whitespace around block content, specifically the empty line before the function itself...

So I'm bringing that up here first, so I can include whatever we decide in that eventual PR. I have my own preference, and I've included arguments for it below, but I'm happy to go with whatever the team votes -- assuming it comes to that.

Options

A: White-space around block content

-- | Return all Assessors for a @'SchoolAdmin'@
--
-- An __Assessor__ is any admin that has assigned at least one district
-- assessment to at least one student accessible to the signed-in admin. This
-- always includes the signed-in admin to avoid a race between school-api (where
-- district assessments are created and fetched) and here (where admins are
-- fetched).
--
getSchoolAdminsAssessorsR :: Handler [SchoolAdminAssessor]
getSchoolAdminsAssessorsR = do

This is what I naturally do without thinking. I like the extra breathing room around all block content (for me, this extends to other formats such as markdown). I think it makes things objectively more readable, but I could just be bias. This rule is also simple to describe, without any special cases, which is usually a benefit in Guides.

I also think there is historical precedent that we prefer:

{-# LANGUAGE -}

module Foo where

And not

{-# LANGUAGE -}
module Foo where

So we like visual separation generally. This feels like a similar decision to me.

B: White-space around block content, except last line

-- | Return all Assessors for a @'SchoolAdmin'@
--
-- An __Assessor__ is any admin that has assigned at least one district
-- assessment to at least one student accessible to the signed-in admin. This
-- always includes the signed-in admin to avoid a race between school-api (where
-- district assessments are created and fetched) and here (where admins are
-- fetched).
getSchoolAdminsAssessorsR :: Handler [SchoolAdminAssessor]
getSchoolAdminsAssessorsR = do

This seems to be what @cdparks and @eborden do naturally without thinking. I will let them add any objective readability points about this themselves, if there are any. The main value to this style is that 2 of the team members do it naturally (vs 1 who does the other).

C: White-space around block content, except first/last line

-- | Return all Assessors for a @'SchoolAdmin'@
-- An __Assessor__ is any admin that has assigned at least one district
-- assessment to at least one student accessible to the signed-in admin. This
-- always includes the signed-in admin to avoid a race between school-api (where
-- district assessments are created and fetched) and here (where admins are
-- fetched).
getSchoolAdminsAssessorsR :: Handler [SchoolAdminAssessor]
getSchoolAdminsAssessorsR = do

Presented without comment. Feels entirely too cramped to me :)

D: One big paragraph

-- | Return all Assessors for a @'SchoolAdmin'@. An __Assessor__ is any admin
-- that has assigned at least one district assessment to at least one student
-- accessible to the signed-in admin. This always includes the signed-in admin
-- to avoid a race between school-api (where district assessments are created
-- and fetched) and here (where admins are fetched).
getSchoolAdminsAssessorsR :: Handler [SchoolAdminAssessor]
getSchoolAdminsAssessorsR = do

I don't think anyone would advocate for this. Just like commit messages, Haddocks are better with a distinct Summary and Body.

E: One big pragraph, hanging indent

-- | Return all Assessors for a @'SchoolAdmin'@. An __Assessor__ is any admin
--   that has assigned at least one district assessment to at least one student
--   accessible to the signed-in admin. This always includes the signed-in admin
--   to avoid a race between school-api (where district assessments are created
--   and fetched) and here (where admins are fetched).
getSchoolAdminsAssessorsR :: Handler [SchoolAdminAssessor]
getSchoolAdminsAssessorsR = do

Again, I don't think anyone wants this, but I've included it for completeness.

Also worth noting: unlike (A) vs (B) vs (C), those vs (D) and (E) will actually change how the rendered documentation looks.

There are other mix-and-match variations between these that I've excluded. Please add them if you want to advocate for them as an option.

Frequencies

Because (a) I write a lot of haddocks and (b) people have accommodated me when I've requested a certain Haddock format in Code Review, (A) is currently the most frequent style across the codebase*

But I'm not sure that's the only factor. I continue to comment on (B), asking it to be (A) in code review so I think if I stopped doing that for a little while (B) would catch up.

* Caveats:

1. This is real hard to measure, so I just did a very rough inexact grep and visual ballpark count
2. This is only if we exclude anything that's not a properly formatted Haddock at all

Discussion vs Voting

Please discuss if you want, of course. But if you want to just vote, throw a 👍 on here for (A) and a 👎 for (B). I will add a vote for myself.

Edited to say "Summary", not "Title"

fomatting typo in best-practies

https://github.com/freckle/guides/blob/main/haskell-best-practices.md

Note that the constructor's argument type doesn't have to match the type argument of the data type (see LessThan and Cond above). The following is perfectly legal (though of dubious utility): ```haskell data Thing a where ThingA :: Int -> Thing Bool ThingB :: Bool -> Thing Int

CREATED PULL REQUEST

Operator-first / Applicative

There are a few styles for Applicative expressions

My personal preference:

parseJSON = withObject "Foo" $ \o -> Foo
  <$> o .: "this"
  <*> o .: "that"

Following a broader (but not documented) "operator-last" guide:

parseJSON = withObject "Foo" $ \o -> Foo <$>
  o .: "this" <*>
  o .: "that"

And each of these comes in a "newlined" flavor:

parseJSON = withObject "Foo" $ \o ->
  Foo
  <$> o .: "this"
  <*> o .: "that"

parseJSON = withObject "Foo" $ \o ->
  Foo <$>
  o .: "this" <*>
  o .: "that"

We of course have a mix across our code-bases now; I'd like to standardize that one way or another.

---

I'm not particularly against operator-last as a general guide, however for expressions such as Applicative, I strongly prefer operator-first. And I believe much of the team agrees.

There are other expressions in the same category, which I would call "structural" expressions. For example, a lens chain like:

foo
  <&> bar .~ baz
  <.> bat .~ quiix

I call these structural because they look like records or lists. And I think a lot of the team (even if operator-last in general) likes operator-first here because it mirrors the comma-first style of records and lists.

This is in contrast to "functional" expressions, which is where I think folks generally prefer operator-last style:

foo . bar . baz $
  bat quix

-- vs
foo . bar . baz
  $ bat quix

---

So, I would like to raise some questions to the team:

1. Would folks be opposed to prefer-operator-first as our overall guide?
2. If so, would folks be opposed to prefer-operator-first for structural expressions?
3. If so, would folks be opposed to prefer-operator-first for Applicative expressions?
4. If so, would folks be opposed to making prefer-operator-last explicit in these guides?
5. If so, that means we're OK letting folks do whatever they personally want, are you sure?
6. If we're making a new guide, are we enforcing the newlined constructor or not?