Refactor Algebra.Solver.*Monoid#2407
Conversation
|
Converted to DRAFT for further refactoring... and now back again. |
|
I'd converted back to draft with a view to more drsatic refactoring, but then lost my nerve over the knock-on consequences for deprecation/backwards-compatibility, so I'll leave those for And in the meanwhile: merge conflict hell! |
|
Will leave the |
JacquesCarette
left a comment
There was a problem hiding this comment.
A couple of minor improvements suggested.
On a bigger scale: this seems like a lot of duplication. Feels like one could abstract over the differences between CommutativeMonoid/Normal and IdempotentCommutativeMonoid/Normal instead of this copy-pasta. But that's an easy comment for me to make!!! I should actually try it.
For sure, the duplication was already there in the library. This PR merely refactors it to make it evident, so in that sense this PR should be regarded as 'provisional', even if mergeable. The The downstream refactoring alluded to in the preamble of #2403 would spruce up the homomorphism correctness proof machinery, but at the cost, again, of having to re-export stuff, now from a different, but again shared, module/ But happy to reconsider, and mark as BTW, part of the purpose of this PR, ahead of tackling the more intricate complexities of |
Well, that's a good question! I guess that the NormalAPI is nowhere exposed as such in the (existing) 'top level' Or not!? |
|
And... as for "compatibility issues", I think there won't be any arising from #2457 ... given the (relative) care taken to preserve (even if to deprecate) the v1.*/v2.1 API ... |
MatthewDaggitt
left a comment
There was a problem hiding this comment.
This looks really good! Added a few small comments, many of which whose causes aren't directly introduced by you.
|
After those are addressed, we should definitely merge! |
|
@MatthewDaggitt thanks for the very detailed feedback. Summarising:
@JacquesCarette Against the discussion on #2457 , which I think can follow-on downstream, I still think this is worth merging now, as a refactoring-with-deprecations. The rest can follow later? |
|
Thanks @MatthewDaggitt for resolving the merge conflict... I'd have got there once I'd woken up... ;-) |
|
Apologies for inaccuracy in my summary of changes: it seems as though I did in fact make the I've suggested on the downstream issue #2472 that we take no action on this (until eg. a release candidate for v2.2?), but advice/counter-suggestions welcome! |
Fixes #2403
NB:
ExprAPI publicly (but not the new API forNormalforms)Monoid.Solveradopts the API for*CommutativeMonoid, but maybe theMonoid.provetype should standardise on this as well?Solvermodules