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 #50

Merged
merged 44 commits into from
Aug 5, 2024

Conversation

jfmengels
Copy link
Contributor

@jfmengels jfmengels commented Jul 25, 2024

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).


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
            -- ...
            ]
          )
        ]
    ]

@mateusfpleite
Copy link
Collaborator

mateusfpleite commented Aug 4, 2024

Thanks a lot for this PR, @jfmengels! The changes have made the API much better and simpler, especially in cases where many rules are needed. I made a few adjustments just to ensure consistency in the documentation.

If @jxxcarlson agrees, I'm happy to merge it.

@mateusfpleite mateusfpleite merged commit 8582510 into jxxcarlson:main Aug 5, 2024
3 checks passed
@jfmengels jfmengels deleted the one-rule-to-rule-them-all branch August 5, 2024 22:11
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