-
Notifications
You must be signed in to change notification settings - Fork 239
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
The free Magma
on a Set
, resp. Setoid
[bis]
#1962
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
I can definitely see how to generate the vast majority of this (given just a few user-provided names).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm a little bit unsure of all these complicated named and unnamed submodules. Generally when they appear it means we haven't got the organisation quite right. It also makes it harder for the user to understand what's in scope at any given point in time.
I wonder how we can simplify it? While doing so we should consider how the organisation will be extended when encoding the free variants of more complicated structures.
I'm now starting to worry about where they should live:
Algebra.Bundles.Free.X or Algebra.Free.X? or somewhere else?
Definitely prefer Algebra.Free.X
Many thanks for all the very helpful and constructive review suggestions, esp. wrt use of private modules. I thinkthat I shouldn't try to add any more to this for now. UPDATED: now added use of |
I agree, and moved things accordingly. |
So... I agree. I think (and hope!) that the use of private modules, and reverting to a more standard naming scheme, now means that the scoping is (much) easier to grasp. But as for the use of nested modules in general, I personally do find it easier to use them to help localise/focus my thinking. And since the constructions are, categorically, unique 'up to', I've definitely found it helpful to keep the various appeals to |
Co-authored-by: G. Allais <[email protected]>
Co-authored-by: G. Allais <[email protected]>
Converting to UPDATED: hopefully the PR is now ready for review after fixing up post-v2.0 merge conflicts plus some cosmetic fixes. |
Can we (now) merge this, please? |
As stand-alone code, this is great. But this also represents a non-trivial new design, and there's various things I am not so sure about. I don't want to merge this in right now, just to have a backwards compatibility problem in 3 months from now. In places, this code seems really quite verbose (and that might be inevitable). And it might benefit quite a lot from the vapourware of a merge of parts of agda-categories into stdlib. And shrink too. So I don't feel like pushing the 'merge' button. I wouldn't be upset of someone else did - I just don't want to be the one to commit this design to the library! |
I'll wait until the discussion concludes on whether to merge at all, and then fix up whatever UPDATED: That said, if not merged (or, in any case!?), then it's probably worth lifting out those commits defining the |
OK, a year has gone by, and even the one positive review is... unclear whether this should be merged. I think it's worth pulling out the bundled |
I agree that splitting off the immediately mergable stuff into a separate PR is a good idea. As to the rest, you'll need to explicitly prod some not-me reviewers into action. |
Marking as |
Converting back to DRAFT:
This PR was only ever intended as a prototype/template for adding all manner of |
Clean version of #1954 .
UPDATED: I'd like to push ahead with this instance, and others, but I'm now starting to worry about where they should live:
Algebra.Bundles.Free.X
orAlgebra.Free.X
? or somewhere else?