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

Quiver #262

Merged
merged 27 commits into from
Aug 13, 2021
Merged

Quiver #262

merged 27 commits into from
Aug 13, 2021

Conversation

JacquesCarette
Copy link
Collaborator

This is a rather large refactor of what used to be Graph into Quiver (as that's really the more appropriate name for this context). It's not necessarily fully done, but I want feedback before I continue.

@JacquesCarette JacquesCarette added the help wanted Extra attention is needed label Apr 4, 2021
@JacquesCarette
Copy link
Collaborator Author

Note that one of the things I'm considering is submitting Quiver (and some other pieces) to stdlib, and maybe move a certain amount of the categorical stuff to categories-examples, as this stuff really doesn't need to be in agda-categories per se.

Copy link
Collaborator

@sstucki sstucki left a comment

Choose a reason for hiding this comment

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

I wrote this "review" in one go, so it documents my confusion at various points, even if some of it subsequently got resolved. The result is a rather long review, sorry about that. I hope it's helpful in showing where the documentation or module structure could be improved. If not, feel free to ignore the comments that turned out to be addressed somewhere else.

Other than that, most of my comments are about documenting or renaming things.
I didn't spot any technical issues.

There are some more philosophical points too. It still feels like quivers don't quite fit into this library, but I'm not sure how this could be solved or if there's a replacement that would feel more natural. Food for thought.

src/Categories/Adjoint/Instance/QuiverCategory.agda Outdated Show resolved Hide resolved
src/Categories/Adjoint/Instance/QuiverCategory.agda Outdated Show resolved Hide resolved
src/Categories/Category/Construction/FreeQuiver.agda Outdated Show resolved Hide resolved
src/Categories/Category/Construction/FreeQuiver.agda Outdated Show resolved Hide resolved
src/Categories/Category/Construction/Quivers.agda Outdated Show resolved Hide resolved
src/Categories/Category/Construction/Quivers.agda Outdated Show resolved Hide resolved
src/Categories/Category/Construction/Quivers.agda Outdated Show resolved Hide resolved
src/Categories/Functor/Construction/FreeCategory.agda Outdated Show resolved Hide resolved
src/Data/Quiver/Morphism.agda Outdated Show resolved Hide resolved
src/Data/Quiver/Morphism.agda Show resolved Hide resolved
@JacquesCarette
Copy link
Collaborator Author

Ok, everything taken care of! This is much nicer now, thanks.

Copy link
Collaborator

@sstucki sstucki left a comment

Choose a reason for hiding this comment

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

LGTM.
There are two changesets where I'm not sure whether they are supposed to be in the PR, though.

src/Categories/Category/Slice/Properties.agda Show resolved Hide resolved
src/Categories/Morphism/Reasoning.agda Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants