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 DataSource.equals() #774

Merged
merged 4 commits into from Mar 27, 2023
Merged

Add DataSource.equals() #774

merged 4 commits into from Mar 27, 2023

Conversation

Starlight220
Copy link
Member

Should fix #720, and maybe even the disconnected source issues.

@@ -116,4 +117,13 @@ public boolean hasClients() {
return !clients.isEmpty();
}

@Override
public boolean equals(Object o) {
return o instanceof DataSource<?> && Objects.equals(((DataSource<?>) o).getId(), this.getId());
Copy link
Member

Choose a reason for hiding this comment

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

Check that the classes are equal, not just instanceof the interface. This might blow up on interactions with destroyed sources

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we want this to interact with destroyed sources?

I think they don't inherit from AbstractDataSource, so they wouldn't have this impl anyway. Should they?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, if they don't extend from this class then I'm fine with the PR as is

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't looked too much at the DestroyedSource impl, but theoretically this comparison might return true if the rhs was a destroyed source with the same URI. If that's possible, is it what we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comparison with a destroyed source in rhs did indeed return true. Changed to check class, and added some tests.

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.

Graphs get duplicate entries on code deploy
3 participants