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

Change Set#delete to return Bool #9590

Merged

Conversation

j8r
Copy link
Contributor

@j8r j8r commented Jul 9, 2020

It adds a simple and efficient way to know if the value was present or not.

Design explanations

Having Set#delete returns object or nil is consistent with other #delete methods in the stdlib:

  • Hash#delete(object) returns the value if object was present.
  • Array#delete_at(index) returns the removed value at the given index, or raises an exception.

However, Deque#delete(object) returns a boolean (maybe it would be better to return a count of deleted objects, but not the topic).

Other options are:

  • add Set#delete?(object) : Bool to have parity with Set#add?, but it can be considered redundant
  • have Set#delete(object) return a Bool. Why not, but could be confusing: #add does not return a Bool (but #add? does)

Side note: I don't think Set#add and Set#add? are redundant, because Set#<< is using Set#add, and one can easily chain object additions like set << one << second << third.

Closes #9591

It adds a simple and efficient way to know if the value was present or not.
@asterite
Copy link
Member

asterite commented Jul 9, 2020

Unless nil is a valid value in the set ;-)

src/set.cr Outdated Show resolved Hide resolved
src/set.cr Outdated Show resolved Hide resolved
@j8r j8r force-pushed the set-delete-return-object-or-nil branch from 570976d to 3859540 Compare July 9, 2020 15:27
j8r and others added 2 commits July 9, 2020 17:32
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
src/set.cr Outdated Show resolved Hide resolved
src/set.cr Outdated Show resolved Hide resolved
j8r and others added 3 commits July 9, 2020 17:53
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
@asterite
Copy link
Member

asterite commented Jul 9, 2020

I think we should make delete return Bool, and remove add? and make add return Bool.

But as usual, I think it's better to discuss these things in an issue, not in a PR.

Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

The API is good, the implementation not.

src/set.cr Outdated Show resolved Hide resolved
@j8r
Copy link
Contributor Author

j8r commented Jul 16, 2020

I have changed Set#delete to return Bool. It is not possible with the current Hash API to return the deleted key reference.

src/set.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
@jhass jhass changed the title Change Set#delete to return object or nil Change Set#delete to return Bool Jul 21, 2020
src/set.cr Outdated Show resolved Hide resolved
Co-authored-by: Jonne Haß <me@jhass.eu>
Copy link
Member

@jhass jhass left a comment

Choose a reason for hiding this comment

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

Fixed outdated specs wording, looks good otherwise

spec/std/set_spec.cr Outdated Show resolved Hide resolved
spec/std/set_spec.cr Outdated Show resolved Hide resolved
Co-authored-by: Jonne Haß <me@jhass.eu>
@straight-shoota straight-shoota added this to the 1.0.0 milestone Nov 22, 2020
@straight-shoota straight-shoota merged commit 1a97e71 into crystal-lang:master Nov 24, 2020
@j8r j8r deleted the set-delete-return-object-or-nil branch November 24, 2020 14:19
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.

Modify Set#delete (and possibily Set#add)
5 participants