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
Additions to method_documentation.rdoc #4065
Additions to method_documentation.rdoc #4065
Conversation
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 appears to be a continuation of #4027. In general, it is a bad idea to close a PR just to submit a similar PR later, as we lose the discussion context of the original PR.
Anyway, I added a couple comments with areas I think should be changed. Other areas look fine.
doc/method_documentation.rdoc
Outdated
- Begin with +new_+ if the output object is a new instance of the receiver's class, | ||
to emphasize that the output object is not +self+. | ||
|
||
A +call-seq+ output may begin with +new_+ for any object that actually is new. |
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.
If we are going to discuss this, we should probably say it should only start with new_
if the object is a new instance of the receiver's class. For example, Hash#to_a
should not use new_array
, it should use array
.
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 with me. but I have a year's worth of the opposite.
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.
Fixed.
doc/method_documentation.rdoc
Outdated
A list should be preceded by and followed by a blank line. | ||
This is unnecessary for the HTML output, but helps in the +ri+ output. | ||
|
||
=== Auto-Links |
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 whether we want to have an Auto-Links section. I think it should be left to the person writing the documentation whether or not they want auto-linking suppressed.
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 agree about "left to the person," but @stomar seemed to have a strong opposing view. I thought it might help if we explicate the point here and reach consensus. Tallyrand reminds us: "If it goes without saying, it goes all the better for saying it."
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 whether we auto-link is important. If the issue is purely visual, @stomar is correct that it could be fixed with styling. It's marginally more of a pain to explicitly avoid auto-linking when writing or editing documentation, so if we have to pick one way or the other, let's just settle it and state that we should not attempt to avoid auto-linking.
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.
Again, ok with me. And again, I'm told after a year's work that what I've been doing is now considered wrong.
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.
Section removed.
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.
Removing the section goes against the discussion we just had. So if you are OK with the consensus to not attempt to avoid auto-linking (as you indicated), you should add the section back and explicitly document that you should not attempt to avoid auto-linking. You should also remove the change from English
to \English
.
Now, as I said previously, I don't think it matters much either way, so if you want to commit this with the section removed, I'm not going to object. But it seems wrong to push for consensus, and then when you don't agree with the consensus, take actions as if the consensus was not reached.
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.
Didn't mean to remove b/c disagreed, meant to remove b/c you'd earlier suggested it. I'll work this out, and thanks.
Should it be ok to suppress when the word (e.g., English, Class) doe not refer to the class?
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.
Should it be ok to suppress when the word (e.g., English, Class) doe not refer to the class?
I hope so 👍
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 agree with @marcandre, if the link doesn't refer to the class, it's OK to suppress auto linking (though not required).
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.
Section now included.
@jeremyevans, now ready for (final?) review. |
Adds: