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

[ refactor ] Change definition of Data.Nat.Base._≤′_ #2523

Merged
merged 4 commits into from
Dec 28, 2024

Conversation

jamesmckinna
Copy link
Contributor

This is exactly analogous to #2504 , in line with #2519 .
Potentially breaking, but all tests pass on stdlib without changes.
Possible downstream improvement for tackling #2442 ?

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.

This seems very reasonable to me.

@MatthewDaggitt MatthewDaggitt added this to the v3.0 milestone Dec 19, 2024
@MatthewDaggitt MatthewDaggitt modified the milestones: v3.0, v2.2 Dec 19, 2024
@jamesmckinna
Copy link
Contributor Author

Re: breaking/v3.0 I guess you're right @MatthewDaggitt although as with #2504 I'm not clear whether this relation has any external use; internally it exists as a mediator for Data.Nat.Induction, moreover one which has been previously refactored extensively so that the single proof involved <⇒<′ already goes via a pattern synonym <′-base... which in fact is (equivalent to) the one introduced by this PR...

... but I should consider uncoupling the last commit to CHANGELOG, as this was made on the assumption that the two PRs would be merged under v2.2...

@MatthewDaggitt
Copy link
Contributor

I havered on this. I mean the previous change was also similarly breaking and got merged. I think the rule we seem to be converging on is that if we break implicit arguments then it is fine?

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Dec 24, 2024

I havered on this.

Fair enough! This relation is actually used in stdlib (but, as usual, we can't speak for external client uses), so I took care to investigate how, and hence whether the consequences of this breaking change would cash out adversely. But I concluded: not.

I mean the previous change was also similarly breaking and got merged. I think the rule we seem to be converging on is that if we break implicit arguments then it is fine?

That's as may be, but AFAIAC, this is purely in the spirit of #2519 : ie. make the reflexivity constructor take a proof of equality explicitly. The implicit/explicit distinction is moot as far as this PR is concerned, I think?

All that said, is it a non-breaking change to replace the (new) pattern synonym with:

pattern ≤′-refl {m} = ≤′-reflexive {n = m} refl

If so, happy to make that change!

src/Data/Nat/Base.agda Outdated Show resolved Hide resolved
@jamesmckinna
Copy link
Contributor Author

The argument for making a breaking change in #2504 was that there were no uses of it in the library, and only one external user (whose PR prompted this whole cycle of refactoring), who could absorb the change without difficulty.

But hopefully the non-breaking suggestion that I make above (which I conjecture is even equivalent to the previous version, because of the way unification works; it might only fail if {n = m} were ever to be supplied explicitly) mops up any 'accidnetal' breakges...?

@JacquesCarette
Copy link
Contributor

These suggestions make sense to me.

@MatthewDaggitt
Copy link
Contributor

Yup, upon reflection happy to merge this either way, but even better if we can make it non-breaking.

@JacquesCarette JacquesCarette added this pull request to the merge queue Dec 28, 2024
Merged via the queue into agda:master with commit ecd3b06 Dec 28, 2024
2 checks passed
@jamesmckinna jamesmckinna deleted the issue2519 branch December 28, 2024 15:24
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