search

jfmengels / elm-review-simplify

Provides elm-review rules to simplify your Elm code
https://package.elm-lang.org/packages/jfmengels/elm-review-simplify/latest/
BSD 3-Clause "New" or "Revised" License
20 stars 9 forks source link

Make checkInfo argument implicit #236

Closed jfmengels closed 9 months ago

jfmengels commented 9 months ago

This is mostly to reduce the re-allocation of the functions that reference it. It also makes the function a little bit shorter.

I've only done it for a (small) part of the code because I'm not sure @lue-bird would be okay with it. What do you think, is this a direction we should go into or not? If so, feel free to merge it, otherwise I'd be curious to discuss it.

github-actions[bot] commented 9 months ago

The branch can be tried out by running:

elm-review --template jfmengels/elm-review-simplify/preview
lue-bird commented 9 months ago

No, this was my plan all along. I find it much cleaner/atomic/readable but I was "worried" about currying overhead. Do you have plans of e.g. changing the -Checks functions into returning a list and applying the firstThatConstructsJust as late as possible?

Feel free to change more of these :)

jfmengels commented 9 months ago

No, this was my plan all along.

Nice :+1:

I find it much cleaner/atomic/readable but I was "worried" about currying overhead.

The currying overhead shouldn't matter too much here. It is slower, but because the functions are defined once at the start of the review application, and the only thing remaining is a function call with the last remaining argument, which is fast because it translates to a plain function call (without A2/A3/...).

Do you have plans of e.g. changing the -Checks functions into returning a list and applying the firstThatConstructsJust as late as possible?

I hadn't thought of that. I'm not sure. I think it could make sense but if it puts the burden on the function that calls the check (and shouldn't necessary know whether there is one or more checks) then I'm not sure it's worth it. But feel free to try it out :+1:

I'll merge this PR now so as not to create conflicts for other things, and let's do these changes as we go along and/or in other PRs like this.