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

Set#to_a& works the same as to_a #14518

Open
meatball133 opened this issue Apr 21, 2024 · 3 comments · May be fixed by #14519
Open

Set#to_a& works the same as to_a #14518

meatball133 opened this issue Apr 21, 2024 · 3 comments · May be fixed by #14519

Comments

@meatball133
Copy link
Contributor

meatball133 commented Apr 21, 2024

The current setup of Set#to_a& has the following example in the docs: # Set{1, 2, 3, 4, 5}.to_a { |i| i // 2 } # => [0, 1, 2], which doesn't seem about right since there should be 5 elements present, as well as looking at the method so doesn't it actually do anything with the accepted block:

def to_a(& : T -> U) : Array(U) forall U
  array = Array(U).new(size)
  @hash.each_key do |key|
    array << key
  end
  array
end

Link to the method in code:

def to_a(& : T -> U) : Array(U) forall U

@ysbaddaden
Copy link
Contributor

Hum, the implementation is indeed wrong, as it never calls the block, but the documentation may still be correct about the intent of filtering duplicates 😕

I tried looking at Ruby, but there's no Set#to_a(&) method.

@ysbaddaden
Copy link
Contributor

An alternative is to deprecate Set#to_a(&) in favor to Set#map(&):

Set{1, 2, 3, 4, 5}.map { |i| i // 2 } # => [0, 1, 1, 2, 2]

Unless we want #to_a(&) to skip duplicates in the array (faster .map(&).uniq), though it's not that intuitive.

@HertzDevil
Copy link
Contributor

It is meant to override Enumerable#to_a(&), I don't think having duplicates in the returned array is problematic in itself

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

Successfully merging a pull request may close this issue.

4 participants