agda / agda-stdlib

The Agda standard library
https://wiki.portal.chalmers.se/agda/Libraries/StandardLibrary
Other
585 stars 237 forks source link

Add `Raw` bundles to `Relation.Binary.*` hierarchy #2491

Open jamesmckinna opened 1 month ago

jamesmckinna commented 1 month ago

This addresses the Relation.Binary.Bundles.Raw part of #2274 ahead of any subsequent refactoring of the Algebra hierarchy in earnest, to exploit the standardisation of vocabulary/fixity envisaged there.

Adds:

NB. Possible issues:

Taneb commented 1 month ago

This looks pretty good, but I do think that the changes to Relation.Binary.Bundles to export these should happen in this PR

jamesmckinna commented 1 month ago

UPDATED: Please ignore this comment!

The latest round of debugging commits suggest that the Eq submodules of Reation.Binary.Bundles.X seem to exist solely to play the 'helper' role envisaged by @JacquesCarette in exporting an extended signature for Setoids with the negated equality symbol... so a 'natural' further refactoring would be to remove all this Eq indirection completely? (At the cost of reifying my difference of opinion with Jacques...).

I'm not clear about the history of these things, but why do we have the Eq submodules factored out in this way? UPDATED: oh, I see that it's one way to ensure that refl and trans as field names don't clash between the underlying isEquivalence and the corresponding IsPreorder properties. I wonder if that was the good/better/best design choice, in favour of having fields names ≲-refl, ≲-trans etc.? Qualified names... hmmm.

jamesmckinna commented 1 month ago

This looks pretty good, but I do think that the changes to Relation.Binary.Bundles to export these should happen in this PR

Now added, and manifest at the top updated to reflect those additions.

JacquesCarette commented 1 month ago

git blame on Relation.Binary.Bundles tells me the first appearance of such a module Eq dates from September 2019 by @MatthewDaggitt back when the file was Relation.Binary.Packages in commit 7e00c92194.

jamesmckinna commented 1 month ago

@JacquesCarette writes:

I'm somewhat undecided if this is the right thing to do. The code seems fine. It is definitely more consistent with what has been done elsewhere, so as a default, this is moving in a coherent direction. I'm just not quite ready to 'approve'.

Beyond your comments on the originating issue #2274 can you say (more) about why you are/might be hesitating? (UPDATED: have just seen your more recent comment there... interesting!)

JacquesCarette commented 1 month ago

Teaching with PLFA right now, have updated to agda-2.7 and stdlib-2.2, and some things have gotten worse. I've got some issues to bring up when I have the time. One example: having Irrelevant show up in some of the goals when working on some stuff to do with Negation was unexpected and unwelcome. There's more.

jamesmckinna commented 1 month ago

Teaching with PLFA right now, have updated to agda-2.7 and stdlib-2.2, and some things have gotten worse. I've got some issues to bring up when I have the time. One example: having Irrelevant show up in some of the goals when working on some stuff to do with Negation was unexpected and unwelcome. There's more.

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2, or even on the PLFA tracker? hard to see how Relation.Nullary.Irrelevant might show up, except via Relation.Nullary.Negation.Core, other than... that should have happened before v2.0...? Shoutout also to @wenkokke who curates PLFA/agda/agda-stdlib compatibility...

JacquesCarette commented 1 month ago

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2,

The example is of the latter kind. But the reason I'm hesitating is related to me finding teaching with post-v2.0 stdlib a bit awkward compared to 1.7.

jamesmckinna commented 1 month ago

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2,

The example is of the latter kind. But the reason I'm hesitating is related to me finding teaching with post-v2.0 stdlib a bit awkward compared to 1.7.

Still: suggest you move this thread off this PR?

wenkokke commented 1 month ago

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2,

The example is of the latter kind. But the reason I'm hesitating is related to me finding teaching with post-v2.0 stdlib a bit awkward compared to 1.7.

Could you submit this as a separate issue, so that we can discuss these issues in detail?

JacquesCarette commented 1 month ago

@wenkokke wrote:

Could you submit this as a separate issue, so that we can discuss these issues in detail?

planning to! I've been swamped, so this is still in my ToDo queue.

MatthewDaggitt commented 1 month ago

I'm not clear about the history of these things, but why do we have the Eq submodules factored out in this way? UPDATED: oh, I see that it's one way to ensure that refl and trans as field names don't clash between the underlying isEquivalence and the corresponding IsPreorder properties. I wonder if that was the good/better/best design choice, in favour of having fields names ≲-refl, ≲-trans etc.? Qualified names... hmmm.

Yes, in retrospect it wasn't the best idea, but at the time, it was the only way to add them that preserved backwards compatibility.

jamesmckinna commented 1 month ago

So, while I would still be happy/happier to see this merged (despite @JacquesCarette 's hesitation about doing so), it's somewhat ironic that we seem, via this route, to have reinstated the 'old' vocabulary (and then erased it via renaming... ;-)), cf. #2099 and its precursor issues/PRs. But I think that each time I/we go round this merry-go-round, things do actually improve?