Skip to content

give writemultinewick same options as writenewick#244

Merged
cecileane merged 4 commits intomasterfrom
dev
Feb 17, 2026
Merged

give writemultinewick same options as writenewick#244
cecileane merged 4 commits intomasterfrom
dev

Conversation

@cecileane
Copy link
Member

also: new method for getconnectingedge, which we would like to use in another package.

@codecov
Copy link

codecov bot commented Feb 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/auxiliary.jl 83.98% <100.00%> (+0.42%) ⬆️
src/readwrite.jl 92.95% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cecileane
Copy link
Member Author

Point of discussion: I proposed a modification of the previously defined method for getconnectingedge, to return nothing instead of throwing an error if the 2 nodes are not connected. That way, both methods behave similarly: they return an edge or nothing. I prefer to return nothing, like findfirst for example, because it makes it easier to handle errors. Instead of needing a try/catch block, we just run the function, then check isnothing() on the result.

Is this a breaking change?

  • Generally no, because getconnectingedge is not exported, currently.
  • It is used in SNaQ, in 2 functions: updateInCycle! and in identifyInCycle,
    but it's not used anywhere else.
  • When used in SNaQ, there is no try/catch block to check for an error. With the proposed change, the resulting edge will be nothing if the 2 nodes are not connected, and the error will occur later in the code -- resulting in an error anyway. The error message will look different, complaining about a nothing object not being usable, rather than an error like "nodes are not connected". The code in SNaQ could remain unchanged (the change won't cause any extra error). Or, later, the code in SNaQ could have an extra line like isnothing(edge) && error("more useful message about the SNaQ status") -- which could be a positive change.

Copy link
Contributor

@NathanKolbow NathanKolbow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good, and I agree with returning nothing from getconnectingedge. Along with your points, nothing also seems like a more intuitive result to me when there is nothing connecting the two nodes.

I was going to add an additional test in test_auxilary.jl for when getconnectingedge is properly returning nothing, but I see that there already is one (line 60), so I'm not sure why the code coverage result shows that line of code as uncovered.

@cecileane
Copy link
Member Author

I was going to add an additional test in test_auxilary.jl for when getconnectingedge is properly returning nothing, but I see that there already is one (line 60), so I'm not sure why the code coverage result shows that line of code as uncovered.

Good point! That's because there are 2 methods, and the nothing case was covered by a test for the new method only, not the existing one. I just added a test --thanks for noticing!

@cecileane cecileane merged commit 14d7b89 into master Feb 17, 2026
6 checks passed
@cecileane cecileane deleted the dev branch February 17, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants