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

Clear Include before updateRecord #3

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

Clear Include before updateRecord #3

wants to merge 25 commits into from

Conversation

amitaibu
Copy link
Owner

No description provided.

@@ -79,6 +72,11 @@ instance Controller LandingPagesController where

setSuccessMessage "LandingPage updated"
redirectTo EditLandingPageAction { .. }
where
extractLandingPageFromInclude :: Include' ["paragraphCtas", "paragraphQuotes"] LandingPage -> LandingPage
extractLandingPageFromInclude landingPage = landingPage
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try to split this into two sub functions: one turning Include' ["paragraphCtas", "paragraphQuotes"] LandingPage -> Include' ["paragraphCtas"] LandingPage and then one turning Include' ["paragraphCtas"] LandingPage -> LandingPage ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That actually works, which isn't intuitive. a9f132a

Why is that (and how did you figure it might work 😄 )?

Is it somehow related to the fact that the compiler thinks that Include "paragraphQuotes" LandingPage is not equal to Include' ["paragraphQuotes"] LandingPage

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this by intuition 😄 In very early IHP versions updateField was actually set and it was later changed (because it was causing errors like these). The core problem with updateField is, that it's type definition is very open. It's defined as updateField :: value' -> model -> model'. Here model and model' are two independent type variables. This means a call to updateField can actually change the return type of the record (e.g. turning LaindingPage to Include "paragraphQuotes" LandingPage). The problem with your first version was that GHC could not figure out the model' variable because multiple updateField were chained (the model type argument is easy for GHC to figure out, it's just that the model' is independent of that).

For comparison set is defined as set :: value -> model -> model. In that case when GHC can figure out model it will never error. So set is typically easy to figure out and unlikely to error. One the other side the problem with set is that it's less flexible and cannot change the output type (e.g. with model = LandingPage it will be set :: value -> LandingPage -> LandingPage, so there's no way to express e.g. the Include stuff). That's why updateField exists.

TL;DR: updateField has a more open type signature than set and sometimes GHC needs a bit of help

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 Thanks for the explanation.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpscholten I've updated Stackoverflow, so this info is not lost :)

https://stackoverflow.com/a/76241756/750039

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

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.

None yet

2 participants