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 yield key in Hash#transform_values and value in #transform_keys #13608

Merged
merged 7 commits into from Jul 22, 2023

Conversation

baseballlover723
Copy link
Contributor

Fixes #13581

spec/std/hash_spec.cr Outdated Show resolved Hide resolved
spec/std/hash_spec.cr Outdated Show resolved Hide resolved
spec/std/hash_spec.cr Outdated Show resolved Hide resolved
Comment on lines 861 to 866
expected_values = h1.values
h2 = h1.transform_keys do |x, v|
v.should eq(expected_values.shift)
x + 1
end
h2.should eq({2 => "a", 3 => "b", 4 => "c"})
Copy link
Member

Choose a reason for hiding this comment

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

Food for thought: Maybe a simpler implementation could combine key and value?

Suggested change
expected_values = h1.values
h2 = h1.transform_keys do |x, v|
v.should eq(expected_values.shift)
x + 1
end
h2.should eq({2 => "a", 3 => "b", 4 => "c"})
h1.transform_keys{ |k, v| "#{k}#{v}" }.should eq({"2a" => "a", "3b" => "b", "4c" => "c"})

The same can be applied to all other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Note: for transform_key! I had to change the type of the hash from Hash(String, Int32) to Hash(String, String) since you can't change the value type in place so that it could use the key value in that spec.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @baseballlover723! Let me add to what was said that the types of the blocks are not respecting the change, and it would be great if the docs can reflect the change too with a little comment.

Let us know if you want us to take over, or if you prefer to take it to completion.

@baseballlover723
Copy link
Contributor Author

@beta-ziliani I'd prefer to take it to completion if possible. To clarify, is what you're asking me to do to update the function defs from def transform_values(& : V -> V2) : Hash(K, V2) forall V2 -> def transform_values(& : V, K -> V2) : Hash(K, V2) forall V2? I was surprised that it worked even without updating the type blocks.

I'll also update the docs as well, though I don't really want to change the example code structure, so the added value will just be unused in the example block.

@beta-ziliani
Copy link
Member

beta-ziliani commented Jun 29, 2023

yes, exactly, that's what I mean. About the documentation, I just meant to add a line stating that the value/key are also yielded to the block. thanks for taking it!

@baseballlover723
Copy link
Contributor Author

@beta-ziliani updated the docs, let me know if the wording is ok with you.

src/hash.cr Outdated Show resolved Hide resolved
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @baseballlover723 !

@straight-shoota straight-shoota changed the title yield key in Hash#transform_values, Hash#transform_values! and yield value in Hash#transform_keys Add yield key in Hash#transform_values, #transform_values! and value in #transform_keys Jul 5, 2023
@straight-shoota straight-shoota changed the title Add yield key in Hash#transform_values, #transform_values! and value in #transform_keys Add yield key in Hash#transform_values and value in #transform_keys Jul 5, 2023
src/hash.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.10.0 milestone Jul 6, 2023
@straight-shoota straight-shoota merged commit 55eefa4 into crystal-lang:master Jul 22, 2023
52 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
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.

Yield key in Hash#transform_values
5 participants