Fix #2396 by removing redundant zero in IsNonAssociativeRing
The zero field in the IsNonAssociativeRing was redundant, and could be replaced with a proof based on the other properties.
Thanks for the speedy follow-up on the issue... but the failing CI tests show that replacing the (redundant) explicit field with a definition/manifest field is breaking behaviour... so we need to tread a bit more carefully!
Issues:
- knock-on consequences: need to update uses/definitions to accommodate the change
-
breakingchanges are generally being punted to v3.0, rather than any v2.x minor version - needs
CHANGELOG(But in view of the version issue, this can, and probably should, wait for the time being...)
Thanks for reviewing this so quickly! I was about to mark the PR as a draft to fix the usage of IsNonAssociativeRing, but you beat me to it.
About the change being breaking and only included in v3.0, makes sense. What is the way forward? Do we leave the MR until we're ready to release the next major version of the library?
I'd definitely go with DRAFT for the time being... I could well imagine some of the other maintainers weighing in on how best to take this forward... and when. But we're a bit more focused on clearing the v2.1 milestone, so this one will definitely be on the back burner for a bit, I think.
Sounds like a plan!
Re: the discussion on #2396 I'd be tempted to take this off DRAFT status now, as it's 'ready to go' (or at least 'ready to review' ;-))... the only problem being that between the next release and v3.0 (hopefully sooner rather than later, but we'll see in the new year), there'll be an almighty CHANGELOG merge conflict, but should be easy to resolve in place then.
@jamesmckinna Updated the PR to be 'ready to go'. I'll tackle the almighty CHANGELOG merge conflict when the time comes...