Skip to content

Conversation

damhiya
Copy link
Contributor

@damhiya damhiya commented Jul 11, 2024

Transitive _#_ condition in the module Relation.Binary.Reasoning.Base.Apartness is unnecessary. Indeed, apartness relation can be transitive only if it is empty, and I think the module name is not appropriate since this module just works for any symmetric relation.

@damhiya damhiya changed the title remove unnecessary condition remove unnecessary condition in the Relation.Binary.Reasoning.Base.Apartness Jul 11, 2024
Copy link
Contributor

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

Agreed - but needs a CHANGELOG entry!

@damhiya
Copy link
Contributor Author

damhiya commented Jul 12, 2024

Is there any opinion about changing the module name?

@MatthewDaggitt
Copy link
Contributor

Sorry for the long reply time.

Is there any opinion about changing the module name?

As always, it's easy to say the current name isn't the best. Do you have a better suggestion we can compare it with? I personally don't think the current name is terrible.

@JacquesCarette would you be willing to approve now?

@MatthewDaggitt MatthewDaggitt added this to the v2.2 milestone Aug 14, 2024
@damhiya
Copy link
Contributor Author

damhiya commented Aug 14, 2024

I recommend Relation.Binary.Reasoning.Base.Symmetric.

Copy link
Contributor

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

I'm fine with this PR doing just the removal of the condition. Name changes can be in a different PR.

@JacquesCarette
Copy link
Contributor

Looking in more detail: what's in the file seems quite consistent with its current name - it is base reasoning about an apartness relation!

@JacquesCarette JacquesCarette added this pull request to the merge queue Aug 14, 2024
Merged via the queue into agda:master with commit b6cf220 Aug 14, 2024
@MatthewDaggitt
Copy link
Contributor

I'm also tempted to agree with @JacquesCarette. It's also not as if it's a very widely used module outside of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants