-
Notifications
You must be signed in to change notification settings - Fork 15
Adding section: What's Here #16
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
Conversation
|
@knu, will you be looking at this? If not, can you suggest another reviewer? |
jeremyevans
left a comment
There was a problem hiding this 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 #+) - |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
Oh, and thanks, @jeremyevans. |
jeremyevans
left a comment
There was a problem hiding this 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], |
There was a problem hiding this comment.
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).
I'm not familiar with rdoc-ref. Will study it, and thanks. |
|
Marking as draft while I look at rdoc-ref usage. |
jeremyevans
left a comment
There was a problem hiding this 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.
Sorry, I was trying to take you off. |
|
@knu, I think this is ready for you. |
knu
left a comment
There was a problem hiding this 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?
|
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! |
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: |
knu
left a comment
There was a problem hiding this 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!
Categorized summary of methods.