-
Notifications
You must be signed in to change notification settings - Fork 46
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
Make behaviour of DigraphRemoveEdge{s}
consistent when removing no edges
#340
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can you also add a test for this behaviour, which verifies that the digraph return by DigraphRemoveEdges
, when given an immutable digraph D
and an empty list, is indeed the identical object to D
.
Please you can you also fixed the spelling mistakes in the commit message, and indeed reformulate the commit message, so that its first line (if possible) consists of 50 or fewer characters?
Oh wait, what is the relationship between this PR and #338? |
@Peter-Ing I agree with @wilfwilson. I think the intended changes would be the following: if
I'm not sure which is preferable: giving an error or just returning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Peter-Ing, I think this PR should be reworked to reflect the comments I made, feel free to leave it open, and just rework your new changes on top of this.
I tend to prefer the former kind of behaviour, I think it is what most people (or at least I) would expect. The latter kind of behaviour seems like it's done for optimisation, letting you reduce code (not having a separate existence check first, in some cases) and save time (by not twice checking whether an edge exists ). Unless users ask for that kind of behaviour, I don't think we should proactively offer it. |
This comment has been minimized.
This comment has been minimized.
DigraphRemoveEdge{s}
consistent when removing no edges
Stale pull request message |
Added an additional method to preserve properties in the case of empty removals.
This addresses issue #260. (Note from Wilf: currently only partially.)