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

feat: add return value for Remove() method #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bychin
Copy link

@Bychin Bychin commented Nov 7, 2024

Add boolean return value for Remove() method.
Sometimes it is useful to know whether the element was actually removed on not. But I don't want to call Contains() before Remove() because it requires extra r-lock call.

@Bychin Bychin changed the title feat: add retrun value for Remove() method feat: add return value for Remove() method Nov 14, 2024
@Bychin
Copy link
Author

Bychin commented Nov 14, 2024

@rdleal may you please look at the PR?

@rdleal
Copy link
Owner

rdleal commented Nov 20, 2024

Hi @Bychin sorry for the delay. I will take a look at the PR today!

@rdleal
Copy link
Owner

rdleal commented Nov 20, 2024

@Bychin the change you are proposing is a breaking change to the current API. We can't change the signature of a function in Go and still keeps its backward compatibility, as users of this package might be relying on the Remove signature to, for example, define an interface on their side:

type PriorityQueueRemover interface {
	Remove(k string)
}

func main() {
	pq := kpq.NewKeyedPriorityQueue[string](cmp) // cmp function definition omitted for brevity.
	var x PriorityQueueRemover = pq
	fmt.Println(x)
}

A good practice here would be to add a new method with the new signature. For example, adding the method Delete(k K) (existed bool) or something like we have in the sync.Map type, PopKey(k K) (value V, exists bool), to remove a specific key, returning its value and a boolean indicating whether it indeed existed in the priority queue.

What do you think of it and which of those you think solves best your use case?

For more on backward compatibility in Go, please refer to the following articles:

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.

2 participants