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

Improve docs on Hash #8887

Merged
merged 3 commits into from
Apr 9, 2020
Merged

Improve docs on Hash #8887

merged 3 commits into from
Apr 9, 2020

Conversation

rdp
Copy link
Contributor

@rdp rdp commented Mar 7, 2020

Tweak Hash docs

Tweak Hash docs
@j8r
Copy link
Contributor

j8r commented Mar 7, 2020

To me it this information was quite obvious: Hash is ordered, as stated already in the object description.

Edit: sorry, I didn't see two different methods description were changed.

src/hash.cr Outdated Show resolved Hide resolved
src/hash.cr Show resolved Hide resolved
@rdp
Copy link
Contributor Author

rdp commented Mar 10, 2020

Yeah it is already fairly obvious, but the other methods called it out so I figured why not :)

attempt give full method signature
@rdp
Copy link
Contributor Author

rdp commented Mar 13, 2020

I think this is ready for review again, thanks!

src/hash.cr Outdated Show resolved Hide resolved
@rdp
Copy link
Contributor Author

rdp commented Mar 30, 2020

I think this is ready for review again, thanks!

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.

I'm not convinced about the type annotation, but 🤷‍♂

@asterite asterite added this to the 0.35.0 milestone Apr 8, 2020
@straight-shoota
Copy link
Member

@asterite What do you mean regarding the type annotation?

@asterite
Copy link
Member

asterite commented Apr 8, 2020

I mean, you usually do hash.each { |k, v|, you don't specify a tuple there because the compiler will expand it. But if there's a type annotation then maybe un experienced users will specify a tuple as a block argument.

Just that feeling.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 8, 2020

Yeah, but we need that signature. Having no signature at all is obviously worse than a signature that could be misunderstood (but really don't lead to any errors).

& : K, V -> _ would techincally work, there shouldn't be a relevant difference (then remove the tuple from yield, obviously), but the tuple signature is required for implementing Enumerable#each(& T : ->). And it's also more correct. We're yielding items, and each item is a tuple of key and value.

@rdp
Copy link
Contributor Author

rdp commented Apr 8, 2020 via email

@asterite
Copy link
Member

asterite commented Apr 8, 2020

There's no need to do anything else here. I'm just waiting on CI.

@rdp
Copy link
Contributor Author

rdp commented Apr 9, 2020

I think CI is stuck FWIW...

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

CI is actually green, just the status hasn't updated.

@straight-shoota straight-shoota merged commit b3a3e1b into crystal-lang:master Apr 9, 2020
@straight-shoota straight-shoota changed the title Update hash.cr Improve docs on Hash Apr 9, 2020
@straight-shoota
Copy link
Member

Thanks @rdp

carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Apr 29, 2020
bcardiff added a commit to bcardiff/crystal that referenced this pull request Jun 10, 2020
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.

None yet

6 participants