Enhanced RDoc for Enumerable - first 5 methods #4025

BurdetteLamar · 2021-01-05T11:47:02Z

Methods:

grep
grep_v
count
find
find_index

I have named this branch '5'; next, containing 5 more, will be '10'.

stomar · 2021-01-05T15:04:07Z

Please separate code blocks and lists (latter does not apply for this PR) by empty lines from the surrounding text, like it is done throughout most parts of the code base.

It doesn't matter for generated HTML, but highly improves readability of generated ri documentation and of the source code itself.

BurdetteLamar · 2021-01-05T15:13:14Z

Please separate code blocks and lists (latter does not apply for this PR) by empty lines from the surrounding text, like it is done throughout most parts of the code base.

It doesn't matter for generated HTML, but highly improves readability of generated ri documentation and of the source code itself.

@stomar: After only, or both before and after?

stomar · 2021-01-05T15:14:49Z

Both (like in the original version).

stomar · 2021-01-05T15:18:07Z

Also, aligning comments for e.g. output values (# => ...) at a specific column can also increase readability, see also the previous version. (Sometimes it makes sense, sometimes not so much.)

enum.c

stomar · 2021-01-05T15:37:43Z

enum.c

+ *   grep(object) -> new_array
+ *   grep(object) {|element| ... } -> new_array
+ *
+ * Returns a new \Array selected by a given object or block.


why "new"? (the receiver needs not be an array)

Isn't the return always a new Array?

It is a new Array 👍

@BurdetteLamar @marcandre

compare to

array.join -> string array.reverse -> new_array

In both cases the returned object is newly created, but "new_thing" is usually used when the receiver is a "thing" and the method returns a different "thing" (and not self).

stomar · 2021-01-05T15:39:13Z

enum.c

+ *   grep(object) -> new_array
+ *   grep(object) {|element| ... } -> new_array
+ *
+ * Returns a new \Array selected by a given object or block.


the block does not select

enum.c

stomar · 2021-01-05T15:55:12Z

I do not yet understand the rigorous escaping of class names, e.g. \Array: it makes sense in the documentation of an array method, but in different modules/classes, a link to the Array class might be useful and would make the docs more discoverable.

BurdetteLamar · 2021-01-05T20:41:29Z

Both (like in the original version).

Fixed.

BurdetteLamar · 2021-01-05T20:41:58Z

Also, aligning comments for e.g. output values (# => ...) at a specific column can also increase readability, see also the previous version. (Sometimes it makes sense, sometimes not so much.)

Fixed.

BurdetteLamar · 2021-01-05T20:45:54Z

I do not yet understand the rigorous escaping of class names, e.g. \Array: it makes sense in the documentation of an array method, but in different modules/classes, a link to the Array class might be useful and would make the docs more discoverable.

I have not changed these. I'm hoping others will comment with more specific guidelines over at method_documentation.rdoc.

BurdetteLamar · 2021-01-05T20:46:47Z

@stomar, I think this is ready for your review.

marcandre · 2021-01-05T23:27:07Z

LGTM 👍

stomar · 2021-01-06T00:02:58Z

enum.c

+ *   grep(pattern) -> new_array
+ *   grep(pattern) {|element| ... } -> new_array
+ *
+ * Returns a new \Array of objects either selected by the given +pattern+


Why "new" both in the call-seq and description (this seems to imply that the receiver is an array and the method returns a new, different array; see my explanation above)?

Why "\Array" and not "Array" (this would produce a helpful link to the Array class)?

"either selected by the given pattern or returned by the block" does not properly describe the behavior; in both cases the selection via the given pattern applies

The returned object is in fact new, regardless of the class of the receiver.

Take this up over at method_documentation.rdoc.

Disagree. Returned Array is in fact either elements selected by the pattern or objects returned by the block. The 1-line summary need not specify on what terms the block is called.

1. yes, they are (of course) new, so no need to mention it. See the explanation already given above:

The established practice is like

array.join -> string array.reverse -> new_array

In both cases the returned object is newly created, but "new_thing" is usually used when the receiver is a "thing" and the method returns a different "thing" (only to make clear that the method dos not return self).

There is no need to write explicitly "string.size returns a new integer ..." and similarly in most of the method docs.

3. Disagree, the selection based on the pattern takes place always, the block only maps/filters the selected elements; the new version can be misunderstood in such a way, that, with block given, no selection happens.

If I thought establish practice was immutable, I wouldn't be doing these revisions.

If you have a suggested revision for the 1-liner, please put it here.

1. These documentations have been improved and tweaked over the course of years, by many contributors; and there are reasons for the common practices that evolved in this process; so lightheartedly waving everything aside with such an argument seems a bit extreme.

As I said, I have an approval for this PR, and so would like to squash and merge.

I agree that array is better than new_array here, since the receiver is not an array.

The decision for Array vs \Array can be made by the person doing the work (so whatever @BurdetteLamar prefers in this case).

either selected by the given pattern or returned by the block is ambiguous, and the literal interpretation does not accurately describe the behavior, as @stomar mentioned. I can't think of a better version that is shorter, but we may want to use a more verbose form, such as selected by the given pattern, and returned by the block if given.

stomar · 2021-01-06T00:14:59Z

I do not have time before tomorrow to have a closer look at the rest of the changes, but I would like to review them also.

BurdetteLamar · 2021-01-06T11:12:08Z

I do not yet understand the rigorous escaping of class names, e.g. \Array: it makes sense in the documentation of an array method, but in different modules/classes, a link to the Array class might be useful and would make the docs more discoverable.

I've looked into this in the existing code. On the one hand, I seem to be the only one doing this; on the other hand, I've been doing this for will over a year (hundreds of instances) and the 12-15 reviewers have no complained. I've opened a PR about this, #4027, so let's take the discussion there to get a consensus.

BurdetteLamar · 2021-01-06T20:25:20Z

I have an approval from @marcandre, and so propose to squash and commit. Anything more can be in a new PR offering further improvements.

stomar · 2021-01-06T22:48:18Z

enum.c

+ *   grep(pattern) -> new_array
+ *   grep(pattern) {|element| ... } -> new_array
+ *
+ * Returns a new \Array of objects either selected by the given +pattern+


Returns an Array of objects selected by the given +pattern+.

(The block version is explained below.)

stomar · 2021-01-06T22:54:02Z

enum.c

+ * Returns a new \Array of objects either selected by the given +pattern+
+ * or returned by the block.
+ *
+ * With no block given, returns a new \Array containing each element


Suggested change

* With no block given, returns a new \Array containing each element

* With no block given, returns an \Array containing each element of the receiver

stomar · 2021-01-06T22:55:47Z

enum.c

- *     res = c.grep(/SEEK/) { |v| IO.const_get(v) }
- *     res                    #=> [0, 1, 2]
+ * With block given,
+ * calls the block with each matching element, returns a new \Array containing each


Suggested change

* calls the block with each matching element, returns a new \Array containing each

* calls the block with each matching element and returns an \Array containing each

stomar · 2021-01-06T22:56:32Z

enum.c

- *     enum.grep(pattern) { |obj| block }  -> array
+ * call-seq:
+ *   grep(pattern) -> new_array
+ *   grep(pattern) {|element| ... } -> new_array


Suggested change

* grep(pattern) {|element| ... } -> new_array

* grep(pattern) {|element| ... } -> array

stomar · 2021-01-06T22:57:20Z

enum.c

- *     enum.grep_v(pattern)                  -> array
- *     enum.grep_v(pattern) { |obj| block }  -> array
+ * call-seq:
+ *   grep_v(pattern) -> new_array


Suggested change

* grep_v(pattern) -> new_array

* grep_v(pattern) -> array

stomar · 2021-01-06T23:15:28Z

enum.c

- *     enum.find(ifnone = nil)                   -> an_enumerator
+ * call-seq:
+ *   find(if_none_proc = nil) {|element| ... } -> obj or nil
+ *   find(if_none_proc = nil) -> new_enumerator


Suggested change

* find(if_none_proc = nil) -> new_enumerator

* find(if_none_proc = nil) -> enumerator

stomar · 2021-01-06T23:19:11Z

enum.c

+ * returns the first element for which the block returns a truthy value:
+ *
+ *   (0..9).find {|element| element > 2}                # => 3
+ *   (0..9).find(proc {false}) {|element| element > 12} # => false


Explanation for if_none_proc follows below, so this example should be moved down.

Missing here: example for return value nil.

stomar · 2021-01-06T23:20:38Z

enum.c

- *     (1..10).find(-> {0})   { |i| i % 5 == 0 && i % 7 == 0 }   #=> 0
- *     (1..100).detect        { |i| i % 5 == 0 && i % 7 == 0 }   #=> 35
- *     (1..100).find          { |i| i % 5 == 0 && i % 7 == 0 }   #=> 35
+ * With no block given, returns a new \Enumerator:


Suggested change

* With no block given, returns a new \Enumerator:

* With no block given, returns an Enumerator:

(Link would be useful.)

stomar · 2021-01-06T23:21:12Z

enum.c

+ * call-seq:
+ *   find_index(object) -> integer or nil
+ *   find_index {|element| ... } -> integer or nil
+ *   find_index -> new_enumerator


Suggested change

* find_index -> new_enumerator

* find_index -> enumerator

stomar · 2021-01-06T23:22:39Z

enum.c

- *     (1..10).find_index  { |i| i % 5 == 0 && i % 7 == 0 }  #=> nil
- *     (1..100).find_index { |i| i % 5 == 0 && i % 7 == 0 }  #=> 34
- *     (1..100).find_index(50)                               #=> 49
+ * With no argument and no block given, returns a new \Enumerator:


Suggested change

* With no argument and no block given, returns a new \Enumerator:

* With no argument and no block given, returns an Enumerator:

stomar · 2021-01-06T23:31:45Z

enum.c

+ *   grep_v(pattern) -> new_array
+ *   grep_v(pattern) {|element| ... } -> new_array
+ *
+ * Returns a new \Array of objects either selected by the given +pattern+


The first paragraph should mention the relation to #grep (inverted); there should also be a link to Enumerable#grep. Having the same description as for grep would be very confusing.

Regarding "either ... or ..." see grep.

BurdetteLamar · 2021-01-07T02:24:49Z

Basta.

BurdetteLamar · 2021-05-03T20:05:33Z

So, @marcandre and @jeremyevans. How can we get things resolved here? This is an important module, deserving of good treatment.

BurdetteLamar · 2021-05-03T20:06:34Z

Should have added: this can't be re-opened (repo no longer exists), but could be put into new PR.

jeremyevans · 2021-05-03T21:23:57Z

Probably best to open a new PR, but please make sure it addresses @stomar's comments.

Enhanced RDoc for Enumerable - first 5 methods

69afabf

BurdetteLamar added the Documentation Improvements to documentation. label Jan 5, 2021

stomar reviewed Jan 5, 2021

View reviewed changes

enum.c Outdated Show resolved Hide resolved

stomar reviewed Jan 5, 2021

View reviewed changes

stomar self-requested a review January 5, 2021 15:51

Enhanced RDoc for Enumerable - first 5 methods

55e0dd6

Enhanced RDoc for Enumerable - first 5 methods

441fb01

marcandre approved these changes Jan 5, 2021

View reviewed changes

stomar reviewed Jan 6, 2021

View reviewed changes

Enhanced RDoc for Enumerable - first 5 methods

ece40c5

BurdetteLamar requested a review from stomar January 6, 2021 17:25

stomar reviewed Jan 6, 2021

View reviewed changes

BurdetteLamar closed this Jan 7, 2021

BurdetteLamar deleted the 5 branch January 7, 2021 02:24

BurdetteLamar mentioned this pull request May 7, 2021

Enhanced RDoc for Enumerable #4473

Merged

	* With no block given, returns a new \Array containing each element
	* With no block given, returns an \Array containing each element of the receiver

	* calls the block with each matching element, returns a new \Array containing each
	* calls the block with each matching element and returns an \Array containing each

	* grep(pattern) {\|element\| ... } -> new_array
	* grep(pattern) {\|element\| ... } -> array

	* find(if_none_proc = nil) -> new_enumerator
	* find(if_none_proc = nil) -> enumerator

	* With no block given, returns a new \Enumerator:
	* With no block given, returns an Enumerator:

	* With no argument and no block given, returns a new \Enumerator:
	* With no argument and no block given, returns an Enumerator:

Enhanced RDoc for Enumerable - first 5 methods #4025

Enhanced RDoc for Enumerable - first 5 methods #4025

Conversation

BurdetteLamar commented Jan 5, 2021 • edited Loading

stomar commented Jan 5, 2021

BurdetteLamar commented Jan 5, 2021

stomar commented Jan 5, 2021

stomar commented Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stomar Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stomar commented Jan 5, 2021

BurdetteLamar commented Jan 5, 2021

BurdetteLamar commented Jan 5, 2021

BurdetteLamar commented Jan 5, 2021

BurdetteLamar commented Jan 5, 2021

marcandre commented Jan 5, 2021

stomar Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stomar Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

BurdetteLamar Jan 6, 2021 • edited Loading

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

stomar commented Jan 6, 2021

BurdetteLamar commented Jan 6, 2021

BurdetteLamar commented Jan 6, 2021

stomar Jan 6, 2021 • edited Loading

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

stomar Jan 6, 2021 • edited Loading

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

BurdetteLamar commented Jan 7, 2021

BurdetteLamar commented May 3, 2021

BurdetteLamar commented May 3, 2021

jeremyevans commented May 3, 2021

BurdetteLamar commented Jan 5, 2021 •

edited

Loading

stomar commented Jan 5, 2021 •

edited

Loading

stomar Jan 6, 2021 •

edited

Loading

stomar Jan 6, 2021 •

edited

Loading

stomar Jan 6, 2021 •

edited

Loading

BurdetteLamar Jan 6, 2021 •

edited

Loading

stomar Jan 6, 2021 •

edited

Loading

stomar Jan 6, 2021 •

edited

Loading