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

Readd Transf #163

Merged
merged 3 commits into from
May 23, 2024
Merged

Conversation

james-d-mitchell
Copy link
Member

No description provided.

@james-d-mitchell james-d-mitchell marked this pull request as draft May 21, 2024 11:55
@james-d-mitchell james-d-mitchell marked this pull request as ready for review May 21, 2024 15:40
@james-d-mitchell
Copy link
Member Author

I think this is ready to go, @Joseph-Edwards

@james-d-mitchell james-d-mitchell added the v1 Label for issues/prs for v1 label May 21, 2024
@Joseph-Edwards
Copy link
Collaborator

Modulo the tests passing (which I assume they will after libsemigroups/libsemigroups#544 is merged) and my minor suggestion above, this looks good to me. That said, I'd like to build and read the docs just to be doubly sure. I'll do this first thing tomorrow morning.

Copy link
Collaborator

@Joseph-Edwards Joseph-Edwards left a comment

Choose a reason for hiding this comment

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

I've read had a more thorough read through the docs now, and it all looks very good to me. I have a few minor comments before I'd be happy to merge.

  1. For consistency, should the new pages in the doc have a table of contents as in Action and Knuth-Bendix?
  2. After the signatures of some of the functions, some return types have hyperlinks and some don't. For example, in PPerm(dom: List[int], img: List[int], deg: int) → StaticPPerm16 | PPerm1 | PPerm2 | PPerm4, StaticPPerm16 gets a hyperlink, but the others don't. It's not immediately clear if there's a nice fix for this (or if a nice fix is even necessary), but it would be nice to do if there is.
  3. If I understood correctly, @james-d-mitchell asked if it was possible to show the constructor when documenting a class. I couldn't remember how to do that at the time, but I now recall that is done with something like this:
.. autoclass:: StaticTransf16
   :class-doc-from: class
   :members:
   :inherited-members:
   :special-members: __init__

With the above comments considered, I would be happy to merge

@james-d-mitchell james-d-mitchell force-pushed the transf-hpp branch 2 times, most recently from 4ed2f52 to dac1a07 Compare May 22, 2024 15:50
@james-d-mitchell james-d-mitchell merged commit d77b042 into libsemigroups:v1 May 23, 2024
11 checks passed
@james-d-mitchell james-d-mitchell deleted the transf-hpp branch May 23, 2024 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1 Label for issues/prs for v1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants