feat: Refactor IGraph to parametric type and extend API support for Graphs.jl integration (#446)#42
feat: Refactor IGraph to parametric type and extend API support for Graphs.jl integration (#446)#42mahmudsudo wants to merge 6 commits intoJuliaGraphs:masterfrom
Conversation
|
Thanks! This seems like a good start, but did you actually run the tests? It seems they fail universally. This should probably be split in 3 separate PRs to help with review and with testing -- (1) interface tests, (2) parametric type, (3) Graphs.jl API |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42 +/- ##
=========================================
- Coverage 5.28% 4.04% -1.25%
=========================================
Files 8 8
Lines 4311 4523 +212
=========================================
- Hits 228 183 -45
- Misses 4083 4340 +257 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Do not worry about the missing parts of the CI -- if you decide to add those, make sure to do these contributions in separate PRs, so that they can be reviewed independently |
by the seperate prs , do you mean the second pr on graphs.jl ? |
|
No, I mean that here in a single PR you have made many unrelated changes. That makes the review slower as one needs to handle many different concerns at the same time. The idea is that 5 small PRs can be reviewed and merged very quickly, while one big PR that has the content of all 5 is much slower to review. |
|
closing as we are now looking at the split PRs |
This PR refactors the core IGraph type to be parametric and extends the API support to satisfy the requirements for full Graphs.jl integration (as part of issue #446).