Open call for improving variable naming in Persistent

[MaxGabriel]

This issue is an open call to improve variable naming in Persistent. Persistent is littered with code that uses extremely short variable names, which makes the code much harder to read. Consider this issue an indication that you can open a PR to improve names anywhere in the codebase.

For the original issue about this, see below:

---

When I read the Persistent codebase, it's often challenging to figure out what is going on, and I attribute a lot of this to very short variable names. I took a few examples from the codebase:

This code is littered with tiny variable names. What is mid exactly?

    (idField, primaryComposite, uniqs, foreigns) = foldl' (\(mid, mp, us, fs) attr ->
        let (i, p, u, f) = takeConstraint ps name' cols attr
            squish xs m = xs `mappend` maybeToList m
        in (just1 mid i, just1 mp p, squish us u, squish fs f)) (Nothing, Nothing, [],[]) attribs

What is n here? And what is rest—rest could be the rest of anything. Is it worth saving a handful of characters to write ps vs persistSettings, which is extremely clear?

takeUniq :: PersistSettings
         -> Text
         -> [FieldDef]
         -> [Text]
         -> UniqueDef
takeUniq ps tableName defs (n:rest)

What is n? nu?

showColumn :: Column -> Text
showColumn (Column n nu sqlType' def _defConstraintName _maxLen _ref) = T.concat
    [ escape n
    , " "
    , showSqlType sqlType'
    , " "
    , if nu then "NULL" else "NOT NULL"
    , case def of
        Nothing -> ""
        Just s -> " DEFAULT " <> s
    ]

In all these cases, you can figure it out, but imo it's way harder than it should be. Here's some code I've written in Persistent I find much easier to read (admittedly this is very biased b/c I wrote it):

requireExtensions :: [[Extension]] -> Q ()
requireExtensions requiredExtensions = do
  unenabledExtensions <- filterM (fmap (not . or) . traverse isExtEnabled) requiredExtensions

getServerVersionNonEmpty :: PG.Connection -> IO (NonEmpty Word)
getServerVersionNonEmpty conn = do
  [PG.Only version] <- PG.query_ conn "show server_version";
  case AT.parseOnly parseVersion (T.pack version) of
    Left err -> throwIO $ PostgresServerVersionError $ "Parse failure on: " <> version <> ". Error: " <> err
    Right versionComponents -> case NEL.nonEmpty versionComponents of
      Nothing -> throwIO $ PostgresServerVersionError $ "Empty Postgres version string: " <> version
      Just neVersion -> pure neVersion

  where
    -- Partially copied from the `versions` package
    -- Typically server_version gives e.g. 12.3
    -- In Persistent's CI, we get "12.4 (Debian 12.4-1.pgdg100+1)", so we ignore the trailing data.
    parseVersion = AT.decimal `AT.sepBy` AT.char '.'

I can totally imagine these functions being harder to read if they used names like ues instead of unenabledExtensions, or neV instead of neVersion for example.

I would propose that Persistent:

1. Use more explicit names
2. Request this in its coding guidelines
3. Be open to PRs to move in this direction.

We could identify code snippets that could be improved, and open an issue for them and tag it as Newcomer to help migrate old code.

I think using shorter names can be fine in some cases, like where:

a. The variable is polymorphic, e.g. map :: (a -> b) -> [a] -> [b], so a long name like element isn't really helping.
b. There is a strong convention, e.g. (x:xs)
c. The scope is very small, e.g. extensionToPragma ext = "{-# LANGUAGE " <> show ext <> " #-}" is short enough that I always have context for what ext is, vs a 15 line function.

But even these can be questionable, e.g. what is c here?

        addParam param val c =
            c { pgConnStr = B8.concat [pgConnStr c, " ", param, "='", pgescape val, "'"] }

I might be swimming against Haskell currents here—I started programming Objective-C which is known for extremely verbose names. Thoughts?

[parsonsmatt]

I am strongly in favor of this. Expanding c to conn would make it pretty obvious what's going on, even!

[MaxGabriel]

Expanding c to conn would make it pretty obvious what's going on, even!

I think c is meant to be conf—it's updating a field of the PostgresConf 😬

data PostgresConf = PostgresConf
    { pgConnStr  :: ConnectionString
   ...
    }

[parsonsmatt]

welp, that does it 😂

[MaxGabriel]

Ok, I would propose we pin this issue, and I edit the top to be an open call for improving the naming of any code in Persistent. Separately, as people see confusing names, they can add specific issues to target a specific snippet of code.