-
Notifications
You must be signed in to change notification settings - Fork 129
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
Make export list explicit instead of re-exporting Hecke++ #2851
Comments
Can you elaborate why we cannot manage breaking/non-breaking releases of Oscar? |
I think it is hard to keep track of our versions like that. Right now we should probably bump the Oscar version to 0.14 since Hecke, Nemo, and AbstractAlgebra had their minor versions bumped and we are re-exporting code that was marked as breaking in those packages. |
I was also confused by the removal of an export in #2846 which turns out to have no effect due to the automatic re-export. |
I totally agree with your reasoning. However, I see some downpoints in doing that:
As an alternative to the problem in 2. we could import AA functions directly, instead of piping everything from AA using im/exports through Nemo and Hecke. Nemo/Hecke need only to import them themselves, if they want to extend it (if they then should re-export it is a different discussion). |
These would then be issues for Nemo/Hecke, but we are dealing with Oscar here. |
After thinking again about this, I don't think that your proposal doesn't change matters for this problem. Oscar has a lot of functions from AA/Nemo/Hecke that only get re-exported without adding any new methods to them. I see two ways for dependents of Oscar to handle these:
|
Yeah I guess strictly speaking that is correct even with the list but then we would at least notice removed exports (via Aqua). |
Either way, the imports need to be cleaned up. Currently we also import a lot of functions manually from AA, Hecke, Nemo that are not necessary due to these loops. (Some of these are necessary, since they are not exported in AA/Hecke/Nemo.) So if the loops stay, these should be removed. Furthermore the loops make developing harder. If I debug something that uses a function coming from AA, I might not even be able to figure out that it comes from there. Let's say I have code using |
Why is this now coming up? Is there a real, existing problem that we're trying to solve? If not: we cannot affort to redo basics just because now we would have done them better - or just differently. But seriously: why? To find weak_popov_with_transform: methods(Oscar.weak_popov_with_transform) anything else is hard:
The argument with "do not now where it's from" is totally moot as this would ONLY cover names introduced and methods provided only in AA (Nemo, ..) any name that is filled in many packages ("*") you'll have to dig and hope anyway that is the price of multiple dispatch. To come back: in Hecke, possibly with the exception of s.th. really fancy, every export should be exported in Oscar. Who is going to maintain the synced import/ export lists across all packages? Unless there is a real benefit, I vote to not change this pattern. It works. |
The benefit would be stability for users of Oscar. That was the main point in my original post.
Right now we have a jump from Hecke 0.20 to 0.22 queued for a 0.13.1 release, Nemo 0.35 to 0.36 So if we want to keep the development model in the current form I think we should not do patch-releases on master (or at least bump the minor version for every minor-version bump of Hecke, Nemo, or AbstractAlgebra). Current master should be 0.14.0-DEV and 0.13.1 will only be done if someone wants to backport some bugfixes to a separate branch. |
Too many packages, the usual problem |
I think we should make the exports of Oscar explicit instead of exporting (almost) everything from Hecke++ automatically.
I guess we could autogenerate a list once and maintain that but we cannot really manage breaking and non-breaking releases for Oscar when we re-export (almost) everything from Hecke, Nemo, and AbstractAlgebra.
Oscar.jl/src/imports.jl
Lines 176 to 180 in aa8e64f
https://github.com/thofma/Hecke.jl/blob/75dbabf5ab7c863bba8fc233a85583b674211554/src/Hecke.jl#L99-L103
https://github.com/Nemocas/Nemo.jl/blob/master/src/Nemo.jl#L58-L64
The text was updated successfully, but these errors were encountered: