Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One rule to rule them all #1

Closed
wants to merge 37 commits into from

Conversation

jfmengels
Copy link
Owner

Hi @jxxcarlson @mateusfpleite

This PR makes it so that every single transformation is not a rule in itself but can be combined into a rule. This makes it possible - or at least easier - to run only a single transformation, without needing to create a whole new project. That makes it easier to run rules from a template.

elm-review --template --fix-all author/package/lamdera --rules AddAuth

(and you can still create multiple templates if you'd like to, both become possible).

It is absolutely fine for you to dismiss this PR/idea, if you think this is a bad idea. I had a bit of fun doing it anyway.

This includes a few bits of cleanup I did prior to the larger API changes. Some are already of note, like stopping the exposing of CustomError which was unnecessary, and that is technically a breaking change. I enabled a few elm-review rules that should help identify issues related to docs and things not being exposed when they should (that wouldn't have caught CustomError though).


Note: I made this PR inside of my fork because it's based off of jxxcarlson#49, but I wanted to get the conversation going. I'll re-do the PR once it's merged (though I think that will lose the comments...).


The API and documentation very likely need a bit of polish. I may have missed updating some documentation, but they APIs could probably be made nicer to use. It's likely a bad idea to publish a new version right after this gets merged.

A few ideas (for which I'd like feedback):

  • Rename Installation to something else that makes more sense. I chose it to get going, but I'm not attached to the name.
  • Name every configuration function config. By the end of the change, I named every function config (Initializer.config, ...) but for the first ones I gave them different names, such as ElementToList.add. I think we could name them all config for consistency, although that would make AddImport.qualified look out of place.

Another idea, which could probably be a bit simpler, is to group installations per module, rather than passing the module name to every single installation. That could look something like this:

config = List Rule
config =
    [ Install.rule "AddPage"
        [ ( "ViewMain"
          , [ Import.qualified [ "Pages." ++ pageTitle ]
              |> Install.Rule.addImport
            -- , ...
            ]
          )
        , ( "Route"
          , [ ElementToList.add "routesAndNames"
                [ "(" ++ pageTitle ++ "Route, \"" ++ routeName ++ "\")" ]
                |> Install.Rule.addElementToList
            -- ...
            ]
          )
        ]
    ]

jxxcarlson and others added 30 commits July 25, 2024 07:14
The value was always 1, so we could inline it and simplify the code.
@jfmengels jfmengels force-pushed the one-rule-to-rule-them-all branch from 0af4484 to 3e612f1 Compare July 25, 2024 11:41
@jfmengels
Copy link
Owner Author

Closing in favor of jxxcarlson#50

@jfmengels jfmengels closed this Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants