fix: fix reading of LGL and NCOL files (broken in 2.0.0)#1347
fix: fix reading of LGL and NCOL files (broken in 2.0.0)#1347aviator-app[bot] merged 4 commits intomainfrom
Conversation
Current Aviator status
This PR was merged using Aviator.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
|
Tried to add unittest for the change in #1348. I'm new to R, so please let me know if there's anything not following common practices. |
|
@krlmlr @Antonov548 One of the CI issues seems to be that GLPK is not found in the macOS image, so it uses the vendored GLPK. |
| ncol_path <- tempfile("testfile", fileext = ".ncol") | ||
| g <- make_graph(c(1, 2, 2, 3)) | ||
| write_graph(g, ncol_path, "ncol") | ||
| expect_no_error(g2 <- read_graph(ncol_path, "ncol")) |
There was a problem hiding this comment.
do we want to know more than no error?
There was a problem hiding this comment.
I think this is good to merge, and the fix should be released ASAP (I'd consider this a very serious bug).
There was a problem hiding this comment.
do we want to know more than no error?
I supposed g and g2 are identical at the beginning, but they are not. The graphs have the same structures but there's some difference in properties. So I replaced expect_true(identical_graphs(g, g2)) with expect_no_error to meet the minimum requirement. If that's not expected, there should be other changes after thorough inspection.
There was a problem hiding this comment.
NCOL will always treat vertex "identifiers" as strings, not numerical vertex IDs, so a difference there is expected.
|
Let me know if there's anything else that you need from me here (it seems to me this is good to go). |
|
Thanks! |
|
This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Once the issues are resolved, remove the Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts). |
4 participants
Fixes #1346
I'm sorry, I really don't have time for a test, but I hope someone can add one based on my first comment in #1346.