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

Remove special case for delta binding since VTX is removed #356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RoukayaZaki
Copy link

@RoukayaZaki RoukayaZaki commented May 20, 2024

Fixes #176


PR-Codex overview

This PR focuses on improving the equalBinding function in Common.hs by ignoring deltas for now.

Detailed summary

  • Improved equalBinding function in Common.hs by ignoring deltas for now
  • Added a TODO to normalize deltas in the future to avoid suppressing problems

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@@ -238,9 +238,6 @@ equalBindings bindings1 bindings2 = and (zipWith equalBinding (sortOn attr bindi

equalBinding :: Binding -> Binding -> Bool
equalBinding (AlphaBinding attr1 obj1) (AlphaBinding attr2 obj2) = attr1 == attr2 && equalObject obj1 obj2
-- Ignore deltas for now while comparing since different normalization paths can lead to different vertex data
-- TODO #120:30m normalize the deltas instead of ignoring since this actually suppresses problems
equalBinding (DeltaBinding _) (DeltaBinding _) = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! But without this line, is equalBinding any different from (==)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe so, because there is equalObject in the AlphaBinding case, which calls equalBindings which sorts the bindings. The Eq instance for Binding wouldn't do that and would call (==) for the objects as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the only difference? @RoukayaZaki may I ask you to check this and add Haddock documentation and doctests to all equal* functions in this module to make sure we do not break it in the future?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I checked and I think it's the only difference. I will doctests soon in a separate PR to double check it.

@deemp
Copy link
Member

deemp commented Jun 21, 2024

@RoukayaZaki, please push a commit (possibly empty) to trigger the CI again.

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.

Common.hs:184: normalize the deltas instead of ignoring...
4 participants