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

Implement ≤-total in terms of _≤?_ #2440

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement ≤-total in terms of _≤?_ #2440

wants to merge 3 commits into from

Conversation

Taneb
Copy link
Member

@Taneb Taneb commented Jul 19, 2024

Fixes #2436

@gallais
Copy link
Member

gallais commented Jul 19, 2024

You'll need a CHANGELOG entry as it's a breaking change for people
who are relying on ≤-total's reduction behaviour on open terms.

@Taneb
Copy link
Member Author

Taneb commented Jul 19, 2024

That's a good point, I'll add a changelog after #2439 gets merged

@JacquesCarette
Copy link
Contributor

I'll wait to do a proper review once the CHANGELOG is in place.

Copy link
Contributor

@jamesmckinna jamesmckinna left a comment

Choose a reason for hiding this comment

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

Modulo:

  • CHANGELOG
  • breaking

This looks great!

@jamesmckinna
Copy link
Contributor

How does this PR contribute to (positively or negatively...) the issues about 'concrete' orderings raised in issue #1179 ?
It seems as though both are trying to tackle a pervasive problem; this PR, locally, with the bigger issue at a (more) global level.

@Taneb
Copy link
Member Author

Taneb commented Aug 30, 2024

@jamesmckinna they're related, but I don't think either of them solve or get in the way of the other

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Aug 30, 2024

Happy to merge once there's a second 'approve' review from one of the other maintainers... and a CHANGELOG entry ;-)

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Sep 1, 2024

I think when you resolve the merge conflict in CHANGELOG, you'll see that your note on this being a non-backwards compatible change has been added to the v2.1 version, and you should bring the whole of that file up-to-date with the current master/v2.2 version...?

@MatthewDaggitt
Copy link
Contributor

The question is do we view this as a backwards compatible breaking change? If it was just a change in behaviour then I would say yes and bump it to v3.0. If however, we argue that this is in fact a performance bug then the maybe we can justify adding it to v2.2?

@Taneb
Copy link
Member Author

Taneb commented Sep 5, 2024

@MatthewDaggitt while I would like it in sooner, of course, the position that has the better argument is that we don't make any performance guarantees at all (our readme says more or less this right near the beginning). Based on that, it probably ought wait for 3.0

@jamesmckinna
Copy link
Contributor

@MatthewDaggitt while I would like it in sooner, of course, the position that has the better argument is that we don't make any performance guarantees at all (our readme says more or less this right near the beginning). Based on that, it probably ought wait for 3.0

Well, if you were to make that sacrifice, that would ease the path to a v2.2 release following #2473 ...

@MatthewDaggitt MatthewDaggitt added this to the v3.0 milestone Sep 28, 2024
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.

Data.Nat.Properties.≤-total should be implemented with _<ᵇ_
5 participants