Improve internal documentation

[begriffs]

Haddock warns about many top level functions and types that lack documentation. Some of these functions speak for themselves, but others would benefit from comments explaining their motivation and context.

Haddock coverage:
   0% (  0 /  9) in 'PostgREST.RangeQuery'
  Missing documentation for:
    Module header
    rangeParse (src/PostgREST/RangeQuery.hs:28)
    rangeRequested (src/PostgREST/RangeQuery.hs:40)
    rangeLimit (src/PostgREST/RangeQuery.hs:50)
    rangeOffset (src/PostgREST/RangeQuery.hs:56)
    restrictRange (src/PostgREST/RangeQuery.hs:44)
    rangeGeq (src/PostgREST/RangeQuery.hs:62)
    allRange (src/PostgREST/RangeQuery.hs:66)
    NonnegRange (src/PostgREST/RangeQuery.hs:26)
   7% (  4 / 54) in 'PostgREST.Types'
  Missing documentation for:
    Module header
    ApiRequestError (src/PostgREST/Types.hs:17)
    DbStructure (src/PostgREST/Types.hs:26)
    PgArg (src/PostgREST/Types.hs:34)
    PgType (src/PostgREST/Types.hs:40)
    RetType (src/PostgREST/Types.hs:42)
    ProcVolatility (src/PostgREST/Types.hs:44)
    ProcDescription (src/PostgREST/Types.hs:47)
    Schema (src/PostgREST/Types.hs:54)
    TableName (src/PostgREST/Types.hs:55)
    SqlQuery (src/PostgREST/Types.hs:56)
    SqlFragment (src/PostgREST/Types.hs:57)
    RequestBody (src/PostgREST/Types.hs:58)
    Table (src/PostgREST/Types.hs:60)
    ForeignKey (src/PostgREST/Types.hs:66)
    Column (src/PostgREST/Types.hs:68)
    Synonym (src/PostgREST/Types.hs:85)
    PrimaryKey (src/PostgREST/Types.hs:87)
    OrderDirection (src/PostgREST/Types.hs:92)
     (src/PostgREST/Types.hs:93)
    OrderNulls (src/PostgREST/Types.hs:97)
     (src/PostgREST/Types.hs:98)
    OrderTerm (src/PostgREST/Types.hs:102)
    QualifiedIdentifier (src/PostgREST/Types.hs:108)
    RelationType (src/PostgREST/Types.hs:114)
    Relation (src/PostgREST/Types.hs:115)
    unPayloadJSON (src/PostgREST/Types.hs:131)
    Proxy (src/PostgREST/Types.hs:134)
    Operator (src/PostgREST/Types.hs:141)
    FValue (src/PostgREST/Types.hs:142)
    FieldName (src/PostgREST/Types.hs:143)
    JsonPath (src/PostgREST/Types.hs:144)
    Field (src/PostgREST/Types.hs:145)
    Alias (src/PostgREST/Types.hs:146)
    Cast (src/PostgREST/Types.hs:147)
    NodeName (src/PostgREST/Types.hs:148)
    SelectItem (src/PostgREST/Types.hs:149)
    Path (src/PostgREST/Types.hs:150)
    ReadQuery (src/PostgREST/Types.hs:151)
    MutateQuery (src/PostgREST/Types.hs:152)
    Filter (src/PostgREST/Types.hs:155)
    ReadNode (src/PostgREST/Types.hs:156)
    ReadRequest (src/PostgREST/Types.hs:157)
    MutateRequest (src/PostgREST/Types.hs:158)
    DbRequest (src/PostgREST/Types.hs:159)
     (src/PostgREST/Types.hs:161)
     (src/PostgREST/Types.hs:177)
     (src/PostgREST/Types.hs:186)
     (src/PostgREST/Types.hs:192)
     (src/PostgREST/Types.hs:195)
   0% (  0 /  7) in 'PostgREST.Error'
  Missing documentation for:
    Module header
    apiRequestError (src/PostgREST/Error.hs:24)
    pgError (src/PostgREST/Error.hs:45)
    simpleError (src/PostgREST/Error.hs:37)
    singularityError (src/PostgREST/Error.hs:55)
    binaryFieldError (src/PostgREST/Error.hs:71)
    encodeError (src/PostgREST/Error.hs:76)
   0% (  0 /  3) in 'PostgREST.DbStructure'
  Missing documentation for:
    Module header
    getDbStructure (src/PostgREST/DbStructure.hs:30)
    accessibleTables (src/PostgREST/DbStructure.hs:164)
 100% (  8 /  8) in 'PostgREST.ApiRequest'
  15% (  2 / 13) in 'PostgREST.QueryBuilder'
  Missing documentation for:
    callProc (src/PostgREST/QueryBuilder.hs:148)
    createReadStatement (src/PostgREST/QueryBuilder.hs:90)
    createWriteStatement (src/PostgREST/QueryBuilder.hs:111)
    getJoinConditions (src/PostgREST/QueryBuilder.hs:390)
    operators (src/PostgREST/QueryBuilder.hs:184)
    pgFmtIdent (src/PostgREST/QueryBuilder.hs:203)
    pgFmtLit (src/PostgREST/QueryBuilder.hs:206)
    requestToQuery (src/PostgREST/QueryBuilder.hs:231)
    requestToCountQuery (src/PostgREST/QueryBuilder.hs:215)
    sourceCTEName (src/PostgREST/QueryBuilder.hs:330)
    unquoted (src/PostgREST/QueryBuilder.hs:333)
 100% (  6 /  6) in 'PostgREST.Auth'
   0% (  0 / 27) in 'PostgREST.Parsers'
  Missing documentation for:
    Module header
    pRequestSelect (src/PostgREST/Parsers.hs:15)
    pRequestFilter (src/PostgREST/Parsers.hs:19)
    pRequestOrder (src/PostgREST/Parsers.hs:29)
    pRequestRange (src/PostgREST/Parsers.hs:36)
    ws (src/PostgREST/Parsers.hs:42)
    lexeme (src/PostgREST/Parsers.hs:45)
    pReadRequest (src/PostgREST/Parsers.hs:48)
    pTreePath (src/PostgREST/Parsers.hs:63)
    pFieldForest (src/PostgREST/Parsers.hs:69)
    pFieldTree (src/PostgREST/Parsers.hs:72)
    pStar (src/PostgREST/Parsers.hs:76)
    pFieldName (src/PostgREST/Parsers.hs:80)
    pJsonPathStep (src/PostgREST/Parsers.hs:91)
    pJsonPath (src/PostgREST/Parsers.hs:94)
    pField (src/PostgREST/Parsers.hs:97)
    aliasSeparator (src/PostgREST/Parsers.hs:100)
    pSimpleSelect (src/PostgREST/Parsers.hs:103)
    pSelect (src/PostgREST/Parsers.hs:110)
    pOperator (src/PostgREST/Parsers.hs:123)
    pValue (src/PostgREST/Parsers.hs:127)
    pDelimiter (src/PostgREST/Parsers.hs:130)
    pOperatiorWithNegation (src/PostgREST/Parsers.hs:133)
    pOpValueExp (src/PostgREST/Parsers.hs:136)
    pOrder (src/PostgREST/Parsers.hs:139)
    pOrderTerm (src/PostgREST/Parsers.hs:142)
    mapError (src/PostgREST/Parsers.hs:158)
   0% (  0 /  4) in 'PostgREST.DbRequestBuilder'
  Missing documentation for:
    Module header
    readRequest (src/PostgREST/DbRequestBuilder.hs:38)
    mutateRequest (src/PostgREST/DbRequestBuilder.hs:247)
    fieldNames (src/PostgREST/DbRequestBuilder.hs:266)
   0% (  0 /  9) in 'Paths_postgrest'
  Missing documentation for:
    Module header
    version (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:28)
    getBinDir (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:39)
    getLibDir (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:39)
    getDynLibDir (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:39)
    getDataDir (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:39)
    getLibexecDir (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:39)
    getDataFileName (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:47)
    getSysconfDir (.stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/autogen/Paths_postgrest.hs:39)
  86% (  6 /  7) in 'PostgREST.Config'
  Missing documentation for:
    PgVersion (src/PostgREST/Config.hs:183)
   0% (  0 /  3) in 'PostgREST.Middleware'
  Missing documentation for:
    Module header
    runWithClaims (src/PostgREST/Middleware.hs:25)
    defaultMiddle (src/PostgREST/Middleware.hs:51)
  25% (  1 /  4) in 'PostgREST.OpenAPI'
  Missing documentation for:
    Module header
    encodeOpenAPI (src/PostgREST/OpenAPI.hs:272)
    pickProxy (src/PostgREST/OpenAPI.hs:293)
   0% (  0 /  2) in 'PostgREST.App'
  Missing documentation for:
    Module header
    postgrest (src/PostgREST/App.hs:65)
Warning: PostgREST.RangeQuery: could not find link destinations for:
    RequestHeaders Range
Warning: PostgREST.Types: could not find link destinations for:
    Text HashMap ToJSON toJSON Value toEncoding Encoding toJSONList toEncodingList Vector Object Header
Warning: PostgREST.Error: could not find link destinations for:
    Response UsageError Status Text ToJSON LByteString toJSON Value toEncoding Encoding toJSONList toEncodingList Error
Warning: PostgREST.DbStructure: could not find link destinations for:
    Session Query
Warning: PostgREST.ApiRequest: could not find link destinations for:
    HashMap Text Request RequestBody
Warning: PostgREST.QueryBuilder: could not find link destinations for:
    Object Query ProcResults Text Value
Warning: PostgREST.Auth: could not find link destinations for:
    HashMap Text Value Secret
Warning: PostgREST.Parsers: could not find link destinations for:
    Text Parser ParseError
Warning: PostgREST.DbRequestBuilder: could not find link destinations for:
    HashMap Text Response
Warning: PostgREST.Config: could not find link destinations for:
    Text Request CorsResourcePolicy
Warning: PostgREST.Middleware: could not find link destinations for:
    Transaction Response Application
Warning: PostgREST.OpenAPI: could not find link destinations for:
    Text LByteString
Warning: PostgREST.App: could not find link destinations for:
    Pool Application

[Skyfold]

I am currently walking through the code and making notes on what each function does or is used for. I started doing this so I can start contributing to the project in a meaningful manner, but I could put together a pull request when I'm done. I just want to know if there is any comment style you prefer or if there is a library you admire for its documentation?

Also, would you be opposed to having a .stylish-haskell.yaml for stylish-haskell? The reason is that QuasiQuotes is a default extension that stylish-haskell needs to know about when formating code. I also found hindent won't parse Main.hs (not exactly sure why).

[Skyfold]

First code question, at the end of Main.hs:

replaceUrlChars = replace "_" "/" . replace "-" "+" . replace "." "="

why do you replace these characters in the JWT secret when configJwtSecretIsBase64 is set to true?

[begriffs]

@Skyfold thanks for volunteering to help with this! Good comments will make a big difference for everyone. I'll review your questions in more detail tonight and will respond then.

[begriffs]

For comment style, adding one for each function is good, like

{-|
  Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
  tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
  veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
  commodo consequat.
-}
lorem :: Ipsum -> Dolor
lorem ...

Example code for a whole module is also nice when it can show how parts fit together. Stuff like:

module Foo.Bar.Baz
  (
    -- * Example usage
    -- $use

    -- * A section
    Thingie(..)

    -- * More stuff
  , fun
  , joy
  ) where

-- now at the bottom of the file:

-- $use
--
-- Here's how to do some stuff
--
-- > this is a raw haskell code block
-- >
-- > more code goes here
--
-- Then another example
--
-- > more fun in this code block
-- > ...

If there is anything noteworthy about individual fields of a data constructor you can document them individually as well like we do for ApiRequest.

Of course the haskell wiki page about comments makes a good point that it's better to fix surprising behavior than to comment it, so keep an eye out for weird things in the code that maybe ought to be fixed too.

For the base64 question, JWT uses base64 with a "URL and Filename Safe Alphabet" aka base64url so we must translate it to regular base64 before decoding.

Also I never heard of .stylish-haskell.yaml but sounds cool so go for it!

[Skyfold]

Thanks for the detailed reply. Do you want to have a header for each file, example?

I am not entirely sure what is happening in the mingw32_HOST_OS #ifndef. I want to say trying to install a handler sigHUP if not on Windows, but I'm not sure why you release the pool.

[begriffs]

Yeah the mingw32_HOST_OS is for conditional compilation. Signal stuff won't compile on windows because AFAICT windows doesn't have signals.

For sigTERM we release the pool because sometimes the connections weren't properly closed (see #268). For sigHUP actually I'm not sure that releasing the pool is required...that's interesting. I guess it hasn't hurt and presumably sigHUP is a manual event that doesn't happen very often.

[steve-chavez]

Some time ago I tried to integrate https://github.com/sol/doctest on some of the functions. It seemed really useful as it could serve as both documentation and unit tests. This would greatly improve clarity in the more complicated modules such as QueryBuilder and DbRequestBuilder.

However at that time, doctest threw a lot of errors and I gave up trying to make it work or to isolate the problem and report an issue.

Maybe the situation has improved and we could give doctest another shot.

[steve-chavez]

Doctests were already introduced on #2019. There's still more doctests to be added, but for better visibility we'll track that on #1804 (pinned issue).