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

Add better error message when trying to get edge that is not present #7245

Merged

Conversation

finsberg
Copy link
Contributor

@finsberg finsberg commented Jan 21, 2024

When trying to get an edge that is not present in the graph it will give a KeyError, but I think it would be better to have an error message saying that the edge is not present in the graph.

@finsberg finsberg force-pushed the add-better-error-messege-edges-getitem branch from 82211cf to 4a29abd Compare January 21, 2024 17:32
@finsberg finsberg marked this pull request as ready for review January 21, 2024 17:34
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I think a better exception message would be an improvement, though changing the exception type would be a backwards-incompatible change. One possible compromise would be to re-raise the KeyError with the new exception message (i.e. replace the nx.NetworkXError in this PR with KeyError).

This would give the better exception message while preserving back compat - WDYT?

@finsberg
Copy link
Contributor Author

finsberg commented Mar 5, 2024

I think a better exception message would be an improvement, though changing the exception type would be a backwards-incompatible change. One possible compromise would be to re-raise the KeyError with the new exception message (i.e. replace the nx.NetworkXError in this PR with KeyError).

This would give the better exception message while preserving back compat - WDYT?

Or, if it is important that all exceptions that are emitted from NetworkX is of type nx.NetworkXError, we could create a new exception that is a subclass of KeyError and nx.NetworkXError, i.e

class NetworkXKeyError(KeyError, nx.NetworkXError)
    pass

@rossbar
Copy link
Contributor

rossbar commented Mar 5, 2024

if it is important that all exceptions that are emitted from NetworkX is of type

IMO this isn't important, but perhaps others have more informed opinions. I'd prefer to stick with builtin exceptions where possible, and I think KeyError works nicely for this case (NX graphs are "dicts all the way down" after all). I can be persuaded otherwise though; the only thing I'm -1 on is a backward-incompatible change in the exception type!

@finsberg finsberg force-pushed the add-better-error-messege-edges-getitem branch from c956934 to 3fc2676 Compare March 6, 2024 09:28
@finsberg finsberg force-pushed the add-better-error-messege-edges-getitem branch from 3fc2676 to 77aa315 Compare March 6, 2024 09:29
@finsberg
Copy link
Contributor Author

finsberg commented Mar 6, 2024

Sounds good. I changed it to re-raise KeyError. Also casting the exception to a string didn't work (see python/cpython#116408 (comment)) so that is why I changed it to use e.value.args instead.

finsberg and others added 2 commits March 6, 2024 17:35
@finsberg finsberg force-pushed the add-better-error-messege-edges-getitem branch from cb91308 to fbf0980 Compare March 6, 2024 16:52
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I took the liberty of pushing a few changes (mostly cosmetic) and tried to simplify the match= logic a bit by making more liberal use of wildcards.

Otherwise this LGTM, thanks @finsberg !

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Looks good... Thanks!

@dschult dschult merged commit 929b5ad into networkx:main Mar 13, 2024
41 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Mar 13, 2024
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
…etworkx#7245)

* Add better error message when trying to get edge that is not present

* reraise KeyError instead for NetworkXError

* Update networkx/classes/tests/test_reportviews.py

Co-authored-by: Dan Schult <dschult@colgate.edu>

* Excape regex pattern

* Minor updates, simplify exception and tests.

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants