Skip to content

Conversation

@BurdetteLamar
Copy link
Member

Categorized summary of methods.

@BurdetteLamar BurdetteLamar added the documentation Improvements or additions to documentation label Apr 14, 2021
@BurdetteLamar BurdetteLamar requested review from hsbt and knu April 14, 2021 17:00
@BurdetteLamar
Copy link
Member Author

@knu, will you be looking at this? If not, can you suggest another reviewer?

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

This seems mostly fine to me, other than changing the linking to use rdoc-ref. However, it probably should be reviewed and approved by @knu.

lib/set.rb Outdated
#
# ### Methods for \Set Operations
#
# - [|](Set.html#method-i-7C) (aliased as #union and #+) -
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think explicitly referencing Set.html is wise. This may work with the default rdoc generator, and possibly other generators, but definitely not all rdoc generators. I think rdoc-ref should be used in such cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Links now fixed (and re-checked).

@BurdetteLamar
Copy link
Member Author

This seems mostly fine to me, other than changing the linking to use rdoc-ref. However, it probably should be reviewed and approved by @knu.

I agree that it should be reviewed and approved by @knu.

@BurdetteLamar
Copy link
Member Author

Oh, and thanks, @jeremyevans.

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

This approach still depends on the specific rdoc generator in use generating ids in a specific format. It would break with an rdoc generator that used a different format. As I already mentioned, rdoc-ref usage seems more appropriate here.

Note that the same issue applies to the internal links to the headings (e.g. Creating a Set). However, rdoc-ref cannot handle those cases, and I guess the links are better than not linking at all.

I don't think I need to review this again, it's up to @knu whether or not to accept it.

lib/set.rb Outdated
# ## What's Here
#
# First, what's elsewhere. \Set includes the module
# {Enumerable}[https://docs.ruby-lang.org/en/master/Enumerable.html],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's a good idea to add this link. Set can be used with multiple Ruby versions, and it's not a good idea to directly link to documentation for a potentially different version (especially the master branch, which may have changes that no released version has).

@BurdetteLamar
Copy link
Member Author

This approach still depends on the specific rdoc generator in use generating ids in a specific format. It would break with an rdoc generator that used a different format. As I already mentioned, rdoc-ref usage seems more appropriate here.

Note that the same issue applies to the internal links to the headings (e.g. Creating a Set). However, rdoc-ref cannot handle those cases, and I guess the links are better than not linking at all.

I don't think I need to review this again, it's up to @knu whether or not to accept it.

I'm not familiar with rdoc-ref. Will study it, and thanks.

@BurdetteLamar BurdetteLamar requested review from jeremyevans and removed request for drbrain, hsbt and jeremyevans April 19, 2021 20:30
@BurdetteLamar BurdetteLamar marked this pull request as draft April 19, 2021 20:31
@BurdetteLamar
Copy link
Member Author

Marking as draft while I look at rdoc-ref usage.

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

@BurdetteLamar If I've already said "I don't think I need to review this again", please do not mark me as a reviewer again.

@BurdetteLamar
Copy link
Member Author

@BurdetteLamar If I've already said "I don't think I need to review this again", please do not mark me as a reviewer again.

Sorry, I was trying to take you off.

@BurdetteLamar
Copy link
Member Author

@knu, I think this is ready for you.

@BurdetteLamar BurdetteLamar marked this pull request as ready for review April 19, 2021 20:56
Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

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

This is nice, but I'm not sure if we should duplicate the descriptions and have two mostly identical sentences for each method. Wouldn't it suffice just to list methods with links in the table of contents?

@knu
Copy link
Member

knu commented Apr 21, 2021

I'm assuming, or hoping that the method names are clear enough in meaning and all you need is the well organized list of methods!

@BurdetteLamar
Copy link
Member Author

This is nice, but I'm not sure if we should duplicate the descriptions and have two mostly identical sentences for each method. Wouldn't it suffice just to list methods with links in the table of contents?

Thanks, @knu. I'd like to leave the 1-liners in. I think it's in the nature of a summary to be repetitive. Also, this format is consistent with the "What's Here" sections for other classes, which I've worked on with @jeremyevans and @marcandre:

@BurdetteLamar BurdetteLamar requested a review from knu April 25, 2021 17:40
Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

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

OK, I appreciate the team efforts. Thanks!

@knu knu merged commit 4ea58a8 into ruby:master Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants