Skip to content

Implement Set#add? method#7495

Merged
straight-shoota merged 2 commits intocrystal-lang:masterfrom
Sija:feature/set-add-question-mark
Mar 11, 2019
Merged

Implement Set#add? method#7495
straight-shoota merged 2 commits intocrystal-lang:masterfrom
Sija:feature/set-add-question-mark

Conversation

@Sija
Copy link
Contributor

@Sija Sija commented Mar 1, 2019

Same as Set#add? from Ruby's stdlib.

chickendan mentioned on a Gitter chat about a need for such API to easily (in one go) distinguish whether upserted value was set before.

@j8r
Copy link
Contributor

j8r commented Mar 1, 2019

Why having this type of methods only for Set, whereas it can be in other Enumerable that implements includes?, like push? and unshift??

No, I don't see a real need to have it in the stdlib. It's already very easy to implement by ourself.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

When a value is added to a Hash you can know whether it existed or not. I think we should avoid that double check somehow (but it means adding some API to Hash)

@Sija
Copy link
Contributor Author

Sija commented Mar 1, 2019

When a value is added to a Hash you can know whether it existed or not.

Can you? I haven't found any way to do that...

(...) (but it means adding some API to Hash)

👍 although I'm curious how to do that when the main method for insert is #[]=...

@Sija
Copy link
Contributor Author

Sija commented Mar 1, 2019

@j8r Because Set is designed with unique values in mind, whereas other Enumerable-compatible classes are not.

@straight-shoota
Copy link
Member

@asterite The hash lookup could be improved, but I suppose we can merge this without it (and maybe add a TODO).

I'm somehow wondering about the return value Set | Nil. It's the same as in Ruby. But why not return Bool? I can't think of a use case where you would have any benefit from returning self.

@asterite
Copy link
Member

asterite commented Mar 1, 2019

Yeah, bool sounds better ai think. And I agree about delaying the optimization.

@Sija
Copy link
Contributor Author

Sija commented Mar 2, 2019

@straight-shoota @asterite I've pushed a commit with proposed changes.

@Sija Sija force-pushed the feature/set-add-question-mark branch from 73ccfc8 to dd7b291 Compare March 2, 2019 08:51
@Sija Sija force-pushed the feature/set-add-question-mark branch from dd7b291 to 57acfc7 Compare March 2, 2019 22:37
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @Sija 👍

@straight-shoota straight-shoota merged commit a2f06ea into crystal-lang:master Mar 11, 2019
@straight-shoota straight-shoota added this to the 0.28.0 milestone Mar 11, 2019
urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 20, 2019
* 'master' of github.com:crystal-lang/crystal:
  Change the font-weight used in Playground (crystal-lang#7552)
  Fix formatting absolute paths (crystal-lang#7560)
  Refactor IO::Syscall as IO::Evented (crystal-lang#7505)
  YAML: fix test checking serialization of slices for libyaml 0.2.2 (crystal-lang#7555)
  Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial comparability (crystal-lang#6611)
  Update `to_s` and `inspect` to have similar signatures accross the stdlib (crystal-lang#7528)
  Fix restriction of valid type vs free vars (crystal-lang#7536)
  Rewrite macro spec without executing a shell command (crystal-lang#6962)
  Suggest `next` when trying to break from captured block  (crystal-lang#7406)
  Fix GenericClassType vs GenericClassInstanceType restrictions (crystal-lang#7537)
  Add human readable formatting for numbers (crystal-lang#6314)
  Add command and args to execvp error message (crystal-lang#7511)
  Implement Set#add? method (crystal-lang#7495)
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.

5 participants