-
Notifications
You must be signed in to change notification settings - Fork 36
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
Improve compatibility with GAP's MatrixObj project #819
Improve compatibility with GAP's MatrixObj project #819
Conversation
I’m happy with this, does it really fix the issues with master? If so, then this seems pretty straightforward and I’ll be happy to merge if you could rewrite the tests ( or I can do this), thanks for the pr @fingolfin |
In my tests, it fixed the issue together with gap-system/gap#4833 (which is not yet merged as it would break our CI). I am not saying this resolve all potential problems, but at least the one at hand. I am at a workshop this week, so I am not sure when I'll be able to look into fixing the tests. If you can do it quickly, that'd be great, otherwise I'll try it once I am free again. |
1e18ed8
to
7726312
Compare
I've discovered a few more places that need But I think I'll also have to disable a few tests, simply because the behaviour in 4.10 / 4.11 will be different from that in 4.12. And arguably some bugs ought to be fixed in GAP master, too... but one step after the other... |
42f280b
to
634f8b8
Compare
@james-d-mitchell I've updated this again (sorry for taking so long). Assuming this PR passes tests, and you are OK with it, I think we'd have to proceed as follows:
|
... by using TryNextMethod in Matrix() methods instead of raising an error
634f8b8
to
224c7b0
Compare
Thanks @fingolfin I'm happy to merge this, and with the rest of the steps too. I'm a wee bit confused why there are 3 commits, when 2 of them are in master already. I tried rebasing and pushing to your branch but it says it's up to date? |
Oh, sorry: this PR is targeting |
Either branch should be ok, maybe rebasing on the present master will be sufficient? |
Changing the base branch is possible with a button press from my phone, so I did that now :-) |
... by using TryNextMethod in Matrix() methods instead of raising an error
Together with gap-system/gap#4833 by @hulpke this should resolve the conflict between GAP master and semigroups. So if we this patch is acceptable for you, then as soon as there is a semigroups release with it, we could merge that GAP PR and finally release GAP 4.12 -- well, after some more testing, obviously! But thanks to the work done on https://github.com/gap-system/PackageDistro we will also (again) get tests of Semigroups (and all other distributed GAP packages) against GAP master, so we can deal with those, too