-
Notifications
You must be signed in to change notification settings - Fork 28
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
Formats outside @require + Package extensions #51
Conversation
5a5cf41
to
f21671f
Compare
Hello. GraphIO.jl has long been waiting for the package extension feature that was shipped with julia v1.9. It is supposed to be backwards compatible. I checked locally with v1.9.0 and v1.8.5. I intend to bring more code here from collapsing NestedGraphsIO.jl that supports both NestedGraphs and MetaGraphs (hopefully MetaGraphsNext at some point too). Remarks
|
I have not had time to get up to date with Julia v1.9, so this is a bit of a learning experience for me. I totally see the point of exporting these symbols even if the necessary extensions are not installed. One thing I wonder now, if there would be an easy way to warn users of this package, if they try to use a symbol where the necessary packages are not installed. Do you have any idea perhaps? Otherwise we might need to update the README so that this becomes at least clear there, we already had quite a few confused users with the current design. Also, I think we should adjust the CI/CD tests, so that the test are run for Julia v1.6 (the minimal version we try to support for Graphs.jl), for Julia v1.9 and for the nightly release. |
f21671f
to
3d2d6bb
Compare
3d2d6bb
to
c55f3ef
Compare
I don't have an obvious solution. I updated the README to reflect what is expected.
I updated the |
Some ideas I have but these could be integrated in a later PR in my opinion: Define a fallback function Theoretically, we could use some metaprogramming as syntactic sugar to automatically load dependencies. |
I agree with the proposal, and I trust @filchristou for the copy-paste ^^ The Requires.jl business in the main file Since this is a big change I'm just gonna add some Aqua testing and formatting to make sure we didn't miss a typo. Then I'll merge |
Subtypes of
AbstractGraphFormat
are not available if the appropriate dependencies are not loaded.I think this is a bad design and we should separate
This leads to problem if a 3rd-party decides to extend the functionality of
AbstractGraphFormat
subtypes.(see https://discourse.julialang.org/t/requires-jl-and-precompilation/53155)
Namely, the new package cannot use the defined types, even if the appropriate dependencies are loaded.
After some small discussion here, I made a an extension package NestedGraphsIO.jl that handles IO for MetaGraphs.jl and NestedGraphs.jl.
However, I need the changes of this PR in order to access the
GraphMLFormat
type defined in GraphIO.jlThe changes here are nothing more that just separating the
@require
from the subtypes definition and the functionality, which I insert in a new file.