Skip to content
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

Change getLink/hasLink to also find link by id #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fecorreiabr
Copy link

@fecorreiabr fecorreiabr commented Oct 7, 2022

This pr is a proposal to improve getLink/hasLink so that it becomes possible to get a link by its id if only one argument is passed to these methods. It seems to work fine in my tests.

With this change it would be possible to get a link both by from-to NodeId and by LinkId:

var graph = createGraph();
var linkId = graph.addLink(1, 2).id; // no need to externally store the link, just its id
...
var link12 = graph.getLink(linkId); // link found by its id
var link12a = graph.getLink(1, 2); // 2 args are passed, keeps current behavior

Maybe this improvement also makes it possible to retrieve the links by id with multigraph option enabled, but I haven't tested this use case yet.

If this is accepted I think docs should be updated to reflect it.

closes #48

@anvaka
Copy link
Owner

anvaka commented Aug 14, 2023

sorry, just noticed this one. I think the need for the method makes sense, though the use of polymorphism with overloaded arguments looks a little bit risky to me: someone might pass second argument incorrectly and the behavior of the method drastically changes.

A bit more secure method that prevents bugs would be an explicit method, e.g. getLinkById(). What do you think?

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.

Get link by id
2 participants