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

Add field access simplification for if/then/else #40

Merged
merged 3 commits into from
Sep 5, 2022

Conversation

miniBill
Copy link
Contributor

Partially fixes #33

src/Simplify.elm Outdated Show resolved Hide resolved
src/Simplify.elm Outdated Show resolved Hide resolved
tests/SimplifyTest.elm Outdated Show resolved Hide resolved
tests/SimplifyTest.elm Outdated Show resolved Hide resolved
@jfmengels jfmengels requested review from jfmengels and removed request for jfmengels August 20, 2022 07:26
@miniBill miniBill marked this pull request as ready for review August 20, 2022 11:36
@miniBill
Copy link
Contributor Author

If merged this would close #33

src/Simplify.elm Outdated Show resolved Hide resolved
@miniBill
Copy link
Contributor Author

Rebased

@jfmengels
Copy link
Owner

Hey @miniBill I'm not sure what the state of this PR is. I don't think the direction this is heading towards is the one. I think that this would make sense if we notice that in every branch, we can end up simplifying the pattern, but not otherwise. What do you think?

@miniBill
Copy link
Contributor Author

It seems like a reasonable restriction! It's uniform, non-surprising, and should have very few false positives!

@miniBill
Copy link
Contributor Author

miniBill commented Sep 5, 2022

Ok, I've restricted the simplification to the case where all leaves are records, record updates or record accesses

@jfmengels
Copy link
Owner

Looks good, thank you very much for your work and patience @miniBill 🙏 😊

@jfmengels jfmengels merged commit 18efc67 into jfmengels:main Sep 5, 2022
@miniBill
Copy link
Contributor Author

miniBill commented Sep 5, 2022

Happy to have it merged ^_^

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.

Distribute field access across ifThenElse, case and letIn
2 participants