adds some inline docs to hash constructors #6923

KCErb · 2018-10-10T10:20:42Z

As described in #6918, here's a little documentation for hash construction.

I decided to nix the initialize docs since the other two self.new methods are wrappers to it. Let me know if that's misguided.

I'm open to adding in docs for other initialization methods (JSON, YAML), I would just need to actually learn about them (haven't used them myself) so it would slow this down a little.

wooster0 · 2018-10-10T17:10:17Z

Run crystal tool format to format your code and fix CircleCI.

jkthorne · 2018-10-10T17:25:19Z

@KCErb Thank you for more docs it is great to have these filled out.
I think integers could be confusing because it looks like array indexing. Using something like Strings might implicitly show that these are different then Arrays.

KCErb · 2018-10-10T17:52:03Z

@r00ster91 Thanks for the pointer, forgot that that runs on doc comments too!
@wontruefree Good point, I changed the examples a bit to address it.

jkthorne · 2018-10-10T18:24:59Z

@KCErb looks good thanks for the docs :)

straight-shoota · 2018-10-10T20:50:38Z

src/hash.cr

@@ -23,10 +24,36 @@ class Hash(K, V)
    @block = block
  end

+  # Creates a new empty `Hash` of *initial_capacity* with a *block* that handles missing keys.


I'd remove of *initial_capacity* from this sentence. It's explained in the next.

straight-shoota · 2018-10-10T20:50:40Z

src/hash.cr

@@ -13,6 +13,7 @@ class Hash(K, V)
  @last : Entry(K, V)?
  @block : (self, K -> V)?

+  # :nodoc:


Why is this nodoc?

I decided to nix the initialize docs since the other two self.new methods are wrappers to it. Let me know if that's misguided.

No, this overload should also be publicly documented.

@KCErb ping

straight-shoota · 2018-10-10T20:51:54Z

src/hash.cr

+  # of the maximum number of elements a hash will hold, the hash should
+  # be initialized with that capacity for improved performance.
+  #
+  # NOTE: *initial_capacity* defaults to 11 and an input of < 11 is ignored.


I don't think this should be a NOTE. Just write that the default and minimal number is 11.

straight-shoota · 2018-10-10T20:52:43Z

src/hash.cr

+  # Creates a new empty `Hash` of *initial_capacity* with a *block* that handles missing keys.
+  #
+  # The *initial_capacity* is useful to avoid unnecessary reallocations
+  # of the internal buffer in case of growth. If you have an estimate


API docs usually don't address the reader directly.

I just copied and pasted this from the Array docs. Not totally sure it's entirely accurate here since I know that bucket size also is used for determining when to rehash. Should I mostly keep it as is but change the voice to avoid "I, me, you" stuff? Should I update the Array docs while I'm at it?

straight-shoota · 2018-10-10T20:53:22Z

src/hash.cr

+  # be initialized with that capacity for improved performance.
+  #
+  # NOTE: *initial_capacity* defaults to 11 and an input of < 11 is ignored.
+  # NOTE: `#size` does not reflect capacity as in `Array`.


Is this comment really useful?

Maybe? It surprised me for just a second before I realized that this makes sense / is obvious, so I decided to make a note in the docs. Happy to remove it.

straight-shoota · 2018-10-10T20:54:08Z

src/hash.cr

+  # NOTE: `#size` does not reflect capacity as in `Array`.
+  #
+  # ```
+  # new_hash = Hash(String, Int32).new(50) do |hash, key|


I would leave the initial capacity out of this example, as it doesn't make sense here.

I would like to include it since it's one of the args to the method I'm trying to demo. I like the example to show exactly how and where to use the argument. But I agree, it's a pretty pointless example. Is there a way to demo with a meaningful example?

straight-shoota · 2018-10-10T21:00:33Z

src/hash.cr

+  # end
+  # new_hash.size # => 0
+  # new_hash["zero"] = 0
+  # new_hash["two"] = 2


These two lines don't really help illustrate the purpose. I think this would be a better example:

hash = Hash(String, Int32).new do |hash, key| hash[key] = key.size end hash.size # => 0 hash["foo"] # => 3 hash.size # => 1 hash["bar"] = 10 hash["bar"] # => 10

Sounds good to me. I was also thinking of borrowing the "go fish" example used in the Ruby docs. Does anyone want to comment on the value of that example as opposed to this one?

They also had one where you used the block feature to essentially use the hash as a cache for some kind of calculation that can be done using the key alone. But I wondered if that was too abstract.

src/hash.cr

KCErb · 2018-10-22T12:34:30Z

Is this one waiting on something from me? I think I've responded to all of the comments. :)

KCErb · 2018-10-26T09:34:38Z

@straight-shoota most recent commit addresses the issues you brought up, thanks for the thorough review.

These initializers overlap in interesting ways, so I was a little conflicted on how best to describe the main purpose of each without being repetitive. Ultimately I decided to move the initialize definition beneath the two new overrides which is probably unorthodox. But I think the result pushes this in the right direction.

RX14 · 2018-10-26T22:43:41Z

@KCErb please don't reorder here - being repetitive and copy/pasting is fine.

KCErb · 2018-11-06T15:07:01Z

Sorry for the slow cycle on this one, I've been much busier than I expected. I restored the order of the initializers and decided to add an example for the first one since it will be the first that a person sees.

straight-shoota · 2018-11-06T15:22:13Z

src/hash.cr

+  # Creates a new empty `Hash` with a *block* for handling missing keys.
+  #
+  # ```
+  # def foo(&block : (Hash(String, Int32), String -> Int32))


This example is too complicated. You don't need a method to demonstrate it. Use the same code as in the next example, but with a proc literal.

straight-shoota · 2018-11-06T15:26:39Z

src/hash.cr

+  # ```
+  #
+  # The *initial_capacity* is useful to avoid unnecessary reallocations
+  # of the internal buffer in case of growth. If the maximum number of elements


initial_capacity should rather be set to the minimum or some number that is very likely to be reached. If you assign the maximum, it could easily lead to regular allocations way beyond the required capacity, hence wasting memory.

Ditto below. And this paragraph should also be added to the third overload.

KCErb · 2018-11-06T16:04:47Z

@straight-shoota thanks for your patience and feedback! I changed the first example to a proc literal as suggested and reworded the note about initial capacity to simply say that if the number of elements is known then this is helpful.

bcardiff · 2018-11-09T03:45:48Z

Thanks @KCErb for ~~improving~~ adding docs here.

Welcome to the contributors list 🎉

adds some inline docs to hash constructors

eb54306

format / improve hash docs

61ac25c

straight-shoota reviewed Oct 10, 2018

View reviewed changes

update Hash initialize docs

0b262e3

restores order of hash initializers, adds example of first initializer

f80a278

straight-shoota reviewed Nov 6, 2018

View reviewed changes

udpate hash constructor docs

7c9aa08

straight-shoota approved these changes Nov 6, 2018

View reviewed changes

RX14 approved these changes Nov 8, 2018

View reviewed changes

RX14 requested a review from bcardiff November 8, 2018 19:51

bcardiff added kind:docs topic:stdlib labels Nov 9, 2018

bcardiff approved these changes Nov 9, 2018

View reviewed changes

bcardiff merged commit c7181be into crystal-lang:master Nov 9, 2018

bcardiff added this to the 0.27.1 milestone Nov 9, 2018

bcardiff added topic:stdlib:collection and removed topic:stdlib labels Jan 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds some inline docs to hash constructors #6923

adds some inline docs to hash constructors #6923

KCErb commented Oct 10, 2018

wooster0 commented Oct 10, 2018

jkthorne commented Oct 10, 2018

KCErb commented Oct 10, 2018

jkthorne commented Oct 10, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

straight-shoota Oct 22, 2018

Sija Oct 22, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

straight-shoota Oct 10, 2018

KCErb Oct 11, 2018

KCErb commented Oct 22, 2018

KCErb commented Oct 26, 2018

RX14 commented Oct 26, 2018

KCErb commented Nov 6, 2018

straight-shoota Nov 6, 2018

straight-shoota Nov 6, 2018

straight-shoota Nov 6, 2018

KCErb commented Nov 6, 2018

bcardiff commented Nov 9, 2018

adds some inline docs to hash constructors #6923

adds some inline docs to hash constructors #6923

Conversation

KCErb commented Oct 10, 2018

wooster0 commented Oct 10, 2018

jkthorne commented Oct 10, 2018

KCErb commented Oct 10, 2018

jkthorne commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KCErb commented Oct 22, 2018

KCErb commented Oct 26, 2018

RX14 commented Oct 26, 2018

KCErb commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KCErb commented Nov 6, 2018

bcardiff commented Nov 9, 2018