Update froidure-pin-hpp for v1/3 #197
Conversation
Joseph-Edwards
force-pushed
the
froidure-pin-hpp
branch
from
b79ac3e to
c3ee8b0
Compare
There was a problem hiding this comment.
All looking good to me. Modulo the suggestions outlined below, I'll be happy to merge. Thanks @james-d-mitchell!
|
|
||
|
|
||
| ######################################################################## | ||
| # Helpers -- from froidure-pin.cpp |
There was a problem hiding this comment.
In some of the helper functions for which we have used @copydoc, the parameters and signatures are incorrect/inconsistent. Some of the functions have signatures with parameter fp: FroidurePin but then the doc says fpb(FroidurePinBase). In other instances, both the signature and the parameter list use FroidurePinBase.
I'm not really sure what the best way to fix this is, if it's worth fixing at all.
One option is to add more parameters to the decorator that makes text substitutions, but maybe that isn't very elegant.
Another option is to not copy the doc, and instead have the doc be very brief and point to the relevant FroidurePinBase function.
There was a problem hiding this comment.
I think I've resolved this, some of the signature should say FroidurePinBase and I just got it wrong when I was writing them in this file. Hopefully caught them all!
Co-authored-by: Joe Edwards <[email protected]>
Co-authored-by: Joe Edwards <[email protected]>
Co-authored-by: Joe Edwards <[email protected]>
Co-authored-by: Joe Edwards <[email protected]>
Co-authored-by: Joe Edwards <[email protected]>
Co-authored-by: Joe Edwards <[email protected]>
Co-authored-by: Joe Edwards <[email protected]>
|
@Joseph-Edwards you happy for me to merge this? |
Yeah looks good to me. Merge away! Thanks @james-d-mitchell! |
This PR requires the corresponding one in libsemigroups to be merged first, and also for some of the tests to be adjusted.