jxxcarlson / elm-review-codeinstaller

Tools for adding code to an existing codebase using `elm-review` rules
MIT License
5 stars 2 forks source link

Revise API #34

Open jxxcarlson opened 1 week ago

jxxcarlson commented 1 week ago

Some ideas on revising the API.

Below are some possible changes presented in a few cases. But whatever we do, we should strive for a coherent API.

Import

We could use the builder pattern even more systematically like this:

Import.config. "Types" [ module "Dict" |> expose ["Dict"]
                       , module "Task"
                       , module "Foo.Bar |> alias "Bar" ]  
                  |> Import.makeRule

Note the change from Import.init to Import.config, which is more descriptive.

Importing a list of unqualified modules can be done via

Import.Config. "Types" [ module "Dict", module "Task", module "Foo.Bar" ]  |> Import.makeRule

One could add to this the construct

Import.unqualified ["Dict", "Task", "Foo.Bar"]

Implementation notes


module : String -> Install.Config
module name = { moduleToImport = name, alias_ = Nothing, exposedValues = Nothing

FieldInTypeAlias

Suppose we change the implementation of our makeRule to

FieldInTypeAlias.makeRule "Types" "FrontendModel" [ "quote: String", "count: Int", "jokes: List String"]

so that the last argument of makeRule is a list. We could, of course, always say

FieldInTypeAlias.makeRule "Types" "FrontendModel" [ "quote: String" ]

to do what we already do with a slight change of syntax.

TypeVariant

Following the suggestions of FieldInTypeAlias and the goal of a coherent API, we could replace the list of rules

    , Install.TypeVariant.makeRule "Types" "BackendMsg" "GotAtmosphericRandomNumbers (Result Http.Error String)"
    , Install.TypeVariant.makeRule "Types" "BackendMsg" "SetLocalUuidStuff (List Int)"
    , Install.TypeVariant.makeRule "Types" "BackendMsg" "GotFastTick Time.Posix"

by the single rule

TypeVariant.makeRule "Types" "BackendMsg" 
    [ "GotAtmosphericRandomNumbers (Result Http.Error String)"
    , "SetLocalUuidStuff (List Int)"
    , "GotFastTick Time.Posix"
  ]

Initializer

Likewise, the list of rules

  , Install.Initializer.makeRule "Backend" "init" "randomAtmosphericNumbers" "Just [ 235880, 700828, 253400, 602641 ]"
  , Install.Initializer.makeRule "Backend" "init" "time" "Time.millisToPosix 0"

could be replaced by the single rule

Initializer.makeRule "Backend" "init" 
  [ "randomAtmosphericNumbers" "Just [ 235880, 700828, 253400, 602641 ]"
  , "time" "Time.millisToPosix 0" 
  ]

Notes

The cumulative effect of these changes would be quite large, .e.g., on the monstrous rule set for magic link authentication. A rough estimate: a reduction from 92 to 11 rules (if we also do something similar for ClauseInCase)

mateusfpleite commented 1 week ago

Really great suggestions! I think we can start working on them right away. Do you think it's better to make a big PR by changing everything at once or proceed module by module?

jxxcarlson commented 1 week ago

It might be best to go module by module. If you would like to take the lead on this, I will review, approve, and merge the PRs as they come in.

jxxcarlson commented 1 week ago

Also: if you would like to select a couple of modules to work on, I can then select a few others so as to work in parallel and speed things up.