Skip to content

[add] Update PartialSetoid reasoning #2689

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

Merged
merged 5 commits into from
Apr 21, 2025

Conversation

Ailrun
Copy link
Contributor

@Ailrun Ailrun commented Apr 3, 2025

Resolves #2677.

This one replaces the previous reasoning type for PartialSetoid with one properly supporting propositional equality steps.

singleStep : ∀ x → x IsRelatedTo x
multiStep : ∀ {x y} (x∼y : x ∼ y) → x IsRelatedTo y
reflexive : ∀ {x y} → x ≡ y → x IsRelatedTo y
relTo : ∀ {x y} (x∼y : x ∼ y) → x IsRelatedTo y
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename constructors to stay closer to Single reasoning

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Apr 4, 2025

Just to check: do these additions/changes fix the issues identified under the original PR?
If so, suggest closing #2677 with a comment to that effect?

@Ailrun
Copy link
Contributor Author

Ailrun commented Apr 4, 2025

@jamesmckinna Yes, this will solve the issue by removing any eager pattern matching on propositional equality proofs. I will update this PR's description so that merging this closes the other PR. edit: The description is now updated.

@jamesmckinna
Copy link
Contributor

This will also need a CHANGELOG entry, as you've changed the API for PartialSetoid reasoning: under Non-backwards-compatible changes, or Bug-fixes? Shoutout to @MatthewDaggitt

@Ailrun
Copy link
Contributor Author

Ailrun commented Apr 7, 2025

@jamesmckinna Changelog entry is added.

Copy link
Contributor

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

Otherwise looks great, thanks for doing this!

@Ailrun
Copy link
Contributor Author

Ailrun commented Apr 10, 2025

@MatthewDaggitt Fixed! Thanks for the comments.

@jamesmckinna
Copy link
Contributor

How to label this one?

  • breaking, hence v3.0
    or
  • bug, hence maybe v2.3

@Ailrun
Copy link
Contributor Author

Ailrun commented Apr 16, 2025

@jamesmckinna It seems like @MatthewDaggitt considers this as a bug fix. I think it is a breaking change, but since no one actually come up with an issue or a PR about the problem in #2677, maybe this one is not so impactful to become a "real" breaking change.

@jamesmckinna
Copy link
Contributor

For me, it's more a case of: let's decide, then merge for v2.3, or defer to the next release...

@Ailrun
Copy link
Contributor Author

Ailrun commented Apr 20, 2025

@jamesmckinna I'm not sure it's okay for me to say this, but let's merge this for v2.3 if no one objects.

@MatthewDaggitt MatthewDaggitt added this to the v2.3 milestone Apr 21, 2025
Copy link
Contributor

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @Ailrun. I'm happy to consider this a bugfix as this was the intended behaviour. Let's merge in v2.3.

@MatthewDaggitt MatthewDaggitt added this pull request to the merge queue Apr 21, 2025
Merged via the queue into agda:master with commit 3880dec Apr 21, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants