Fix(graph): Allow any Line subclass as edge_type in Graph/DiGraph

[Akshat-Mishra-py]

Overview: What does this pull request change?

Fix #4250

More broadly, this fix allows any subclass of Line to be used as an edge_type in the Graph and DiGraph, making the implementation more flexible and extensible.

Motivation and Explanation: Why and how do your changes improve the library?

Previously, the Graph and DiGraph classes assumed that the edge_type will always have its first two arguments as start and end point.
But, such is not true for subclasses of Line. This change lets user use any subclass of Line in Graph and DiGraph.

Changes

• Refactored Graph and DiGraph:
  • Fixed edge handling logic to support all subclasses of Line, not just LabeledLine.
  • Modified _add_edge accordingly.
• Testing:
  • Added/updated tests for LabeledLine as edge types in test_graph.py.

Reviewer Checklist

• The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• If applicable: newly added functions and classes are tested

[mdrost]

Was it really unused? Maybe some static analysis tool was using it?

[mdrost]

Anyway I think it should not belong to this PR. Lets keep things clean.
Just do git rebase and remove the commit, instead of doing revert commit.

[Akshat-Mishra-py]

mypy was failing due to this.

[mdrost]

Oh, I need to teach myself what is mypy :P

[Akshat-Mishra-py]

mypy is failing on my local system because I am using windows which can be ignored now that I think I would need to reintroduce the comment to pass the checks

[mdrost]

I think this PR should not be solely about supporting LabeledLine but be about generic support of classes based on Line as author intended in the Graph API.
As such it would be nice to test more classes inheriting from Line e.g. DashedLine and maybe something more for completeness. Here we could have more generic test that loops around map of classes with its arguments. Or maybe unit test framework support some more sophisticated parameterized tests, I don't know.
Maybe do git rebase and squish commits with fixes and tests, to avoid explosion of commits in the history.

Nice Work.

[Akshat-Mishra-py]

I think this PR should not be solely about supporting LabeledLine but be about generic support of classes based on Line as author intended in the Graph API. As such it would be nice to test more classes inheriting from Line e.g. DashedLine and maybe something more for completeness. Here we could have more generic test that loops around map of classes with its arguments. Or maybe unit test framework support some more sophisticated parameterized tests, I don't know. Maybe do git rebase and squish commits with fixes and tests, to avoid explosion of commits in the history.

Nice Work.

I do understand that this PR fixes all the Line subclass to be used in Graph and DiGraph but for checking every subclass of Line should i create a new test file from scratch or include it in the same test_graph.py or create a new test file I can use Line.__subclasses__() to automate the tests but we will need to import all the subclasses manually to create such a test as Line.__subclasses__() this will not List subclasses if they are not manually imported first.

[mdrost]

I can use Line.__subclasses__() to automate the tests but we will need to import all the subclasses manually to create such a test as Line.__subclasses__() this will not List subclasses if they are not manually imported first.

Testing every class from Line.__subclasses__() would be impossible as each class requires own set of edge_config arguments.

should i create a new test file from scratch or include it in the same test_graph.py

I think it would be enough to just test few more classes inside test_graph.py def test_graph_accepts_labeledline_as_edge_type (and rename the test accordingly) such as DashedLine and DoubleArrow that seems to be good candidates to be regularly used by users.

btw. Accepting PR is not up to me and as such you might need to wait some time for main developers.

[chopan050]

LGTM! Thanks for allowing the use of v0.19.0's new LabeledLine in graphs.

Technically, the rest of Line's subclasses, such as DashedLine, Arrow and DoubleArrow, are fine, because their first parameters were also start and end. Regardless of that, it's always better to be more explicit.

If possible, could you please also add tests for some other Line subclasses as mdrost suggested, just in case?

[Akshat-Mishra-py]

LGTM! Thanks for allowing the use of v0.19.0's new LabeledLine in graphs.

Technically, the rest of Line's subclasses, such as DashedLine, Arrow and DoubleArrow, are fine, because their first parameters were also start and end. Regardless of that, it's always better to be more explicit.

If possible, could you please also add tests for some other Line subclasses as mdrost suggested, just in case?

Ok I will do that and push the changes, thank you for reviewing.

[behackl]

Thank you very much!