-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feature record update #337
base: master
Are you sure you want to change the base?
Conversation
it "works with RecordUpdate" $ do | ||
uses (named "f") (RecordUpdate (Reference "f") [("g", Reference "h")]) `shouldBe` True | ||
uses (named "h") (RecordUpdate (Reference "f") [("g", Reference "h")]) `shouldBe` True | ||
uses (named "g") (RecordUpdate (Reference "f") [("g", Reference "h")]) `shouldBe` False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not married to this. See description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure, too.
I would take the same path as with assignments. If we are considering assignment as usage, then should we consider record update's assignee as a usage too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks quite good. My main concerns are about the visitor module: we should at least update the documentation of the visit pattern.
However, I am unsure about the motivation of this feature. Are we actually going to introduce new expectations for this RecordUpdate
? Or do we just need the inner contents of the record update to be parsed? May an Other
here also solve the problem?
it "works with RecordUpdate" $ do | ||
uses (named "f") (RecordUpdate (Reference "f") [("g", Reference "h")]) `shouldBe` True | ||
uses (named "h") (RecordUpdate (Reference "f") [("g", Reference "h")]) `shouldBe` True | ||
uses (named "g") (RecordUpdate (Reference "f") [("g", Reference "h")]) `shouldBe` False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure, too.
I would take the same path as with assignments. If we are considering assignment as usage, then should we consider record update's assignee as a usage too
normalizeSwitchCases ops = map (\(e1, e2) -> (normalize ops e1, normalize ops e2)) | ||
normalizeRecordUpdates ops = map (\(i, e) -> (i, normalize ops e)) | ||
normalizeTryCases ops = map (\(p, e) -> (p, normalize ops e)) | ||
normalizeSwitchCases ops = map (\(e1, e2) -> (normalize ops e1, normalize ops e2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could generalize those tree functions now. The first two are the same, and are mostly the same in all files.
Agree |
I am unsure about this one, too. Do we have any other examples in other languages? In any case, rename and replace don't currently target record's field names. |
I... I forgot |
Fixes #336.
Thought of using
FieldAssignment
at first but got dissuaded by the fact that you can update multiple fields per record update.I didn't use an expression for the update's field name as it's not quite the same as other
Reference
s (i.e. it cannot be parameterized). That being said, in retrospective, that might be too strongly tied to haskell's way of doing it? How is this supposed to work with rename and replace? Not so sure about this one!