-
Notifications
You must be signed in to change notification settings - Fork 111
initial graph properties API support #464
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
base: master
Are you sure you want to change the base?
Conversation
b979c82
to
47ff6af
Compare
This can not be merged until the GraphProperties.jl package is registered. If the design looks OK I will start the registration process for GraphProperties.jl eventually. |
This comment was marked as outdated.
This comment was marked as outdated.
Julia Formatter in the tests uses v1 (check the project.toml in tests). We have not converted to v2 yet. |
To me it seems like an interesting proposition, but I suspect it will take a while to get the input of all stake holders. I am more of a caretaker of the package, as a lot of the people that actually built the package are not active maintainers anymore, so I would like to be slow and methodical in gathering feedback on this change. While it is not breaking in any way, and it seems well thought out, going in this direction will be a significant future API commitment. Here are initial questions that will pop up in the discussion about this (addressing some of them early on will probably save us some time when we start gathering feedback on various forums):
|
8a0a503
to
3d320a1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #464 +/- ##
==========================================
+ Coverage 97.38% 97.64% +0.25%
==========================================
Files 120 118 -2
Lines 7004 6918 -86
==========================================
- Hits 6821 6755 -66
+ Misses 183 163 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Just popping by to say that adding a new dependency to Graphs, especially one registered so recently, is bound to raise some eyebrows, especially without justification. As Stefan pointed out, there have been many many redesign discussions which eventually led nowhere for lack of dev time, so I like his conservative approach to caretaking for the time being. |
motivation, for the PR and for the new package, to be added soon
EDIT: putting this on hold, may still need to think about the design of GraphProperties.jl