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

removal of potentially harmful parameter. #91

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Vithanco
Copy link

@Vithanco Vithanco commented Jan 7, 2025

I managed to build an inconsistent graph as the edge.directed information was overwritten by 2nd parameter of addEdge(_, directed:). Now, the directed parameter is no longer needed. Instead, the information is taken from the Edge parameter.
One source of potential problems less.

I managed to build an inconsistent graph as the edge.directed information was overwritten by addEdge(_, directed:). Now, the directed parameter is no longer needed. Instead, the information is taken from the Edge parameter.
@Vithanco
Copy link
Author

Vithanco commented Jan 7, 2025

I should mention, that I only removed the parameter where the information is already provided via Edge. In other cases, like convience addEdge methods, it is kept.

@Vithanco
Copy link
Author

Vithanco commented Jan 7, 2025

This is, unfortunately, a breaking change as I changed the Graph protocol.

@davecom
Copy link
Owner

davecom commented Jan 8, 2025

Thanks for the contribution. This does potentially make sense but yeah unfortunately is a breaking change. One way to make it a non-breaking change would be to have new one parameter methods for addEdge() as you provided but keep the older ones in place so as not to break compatibility. Let's leave this up for a little bit and see if anyone has any comments.

@davecom davecom added this to the 3.2 milestone Jan 8, 2025
@Vithanco
Copy link
Author

Vithanco commented Jan 8, 2025

I think I managed to make it a non-breaking change for most use cases. The "only" time the code will now break is if someone else implemented the Graph protocol separately.
In addition, I was able to mark the potentially problematic method with a deprecated annotation.

Copy link
Contributor

@ZevEisenberg ZevEisenberg left a comment

Choose a reason for hiding this comment

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

A couple of minor suggestions on the last commit, but this seems like a smart change, and the deprecation is a nice way to go about it.

@@ -39,6 +39,11 @@ extension Graph {
public var edgeCount: Int {
return edges.joined().count
}

@available(*, deprecated, renamed: "addEdge", message: "Use the addEdge method without the additional directed parameter instead, as the Edge contains already the information about direction. A double specification can only result in inconsistencies and errors.")
func addEdge(_ e: E, directed: Bool = false){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func addEdge(_ e: E, directed: Bool = false){
func addEdge(_ e: E, directed: Bool = false) {

Copy link
Author

Choose a reason for hiding this comment

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

actually, you found an outdated version. I removed the default value (false) in the latest version.

Copy link
Author

Choose a reason for hiding this comment

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

Without the default value there is a more clear distinction between the one-parameter version and the 2-parameter. And there is no confusion that you use the one-parameter version when you leave out the default parameter - and not using the default value

Copy link
Author

Choose a reason for hiding this comment

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

formatting updated.

Vithanco and others added 3 commits January 8, 2025 17:06
@Vithanco
Copy link
Author

Vithanco commented Jan 9, 2025

sorry, KneupnerTrackunit is my alterEgo. Not sure why the last commits where under that name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants