Skip to content
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

Fix missing rdoc for Gem::Version #5299

Merged
merged 1 commit into from Feb 7, 2022

Conversation

nevans
Copy link
Contributor

@nevans nevans commented Jan 21, 2022

The rdoc for Gem::Version is available here:

However it is currently missing from:

The rdoc for Gem::Version is available here:
* https://docs.ruby-lang.org/en/3.0/Gem/Version.html

However it is currently missing from:
* https://ruby-doc.org/stdlib-3.1.0/libdoc/rubygems/rdoc/Gem/Version.html
* https://docs.ruby-lang.org/en/3.1/Gem/Version.html
* https://docs.ruby-lang.org/en/master/Gem/Version.html
* `ri Gem::Version`
  with `ri --version` => 6.4.0 and `gem --version` => 3.3.5
* `yard ri Gem::Version` with `yard --version` => 0.9.27
@welcome
Copy link

welcome bot commented Jan 21, 2022

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

@deivid-rodriguez
Copy link
Member

Thank you @nevans! Any ideas on how to protect ourselves from accidentally regressing again here?

@nevans
Copy link
Contributor Author

nevans commented Jan 23, 2022

Any ideas on how to protect ourselves from accidentally regressing again here?

That's a great question, @deivid-rodriguez. I've never tested comments/documentation before, and generally I'd be wary of it. But for critical core libraries like this, a simple set of smoke-screen tests could be very useful? Documentation is a user-facing feature, too!

I've never called rdoc programmatically (not counting rake) so my first thought is: we could fire up rdoc inside the test suite and ensure the appropriate Gem::Version docs are output to the html or ri files? My second thought after a quick scan of the rdoc docs: perhaps run something like rdoc = RDoc::RDoc.new.parse_file(path) and traverse the RDoc::CodeElements until we find what we're looking for?

Maybe there's an example we can emulate from rdoc's own tests?

What do you think?

@nevans
Copy link
Contributor Author

nevans commented Jan 23, 2022

Or simpler, maybe a Rubocop rule could have caught this?

@deivid-rodriguez
Copy link
Member

Those are all good ideas. I was also just thinking of RuboCop, yeah, sounds like something fairly simple to check. But I was also wondering, why did this change break docs? Maybe it would also be possible to enhance RDoc to behave better in this situation and still generate documentation?

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Jan 23, 2022

why did this change break docs?

Some doc systems look to the next code line to determine what 'object' (class/module/constant/method) to associate the comment with. A require_relative statement doesn't get doc'd, so the comment is ignored. Some also allow no more than one blank line between the comments and the 'object'.

I'm going off of memory, haven't checked. If this was otherwise, there would have to be a lot more 'look-ahead' parsing to determine layout...

@nevans
Copy link
Contributor Author

nevans commented Jan 23, 2022

IMO rdoc was correct: lacking any explicit metadata to the contrary, the comment looked like it applied to the require not the class. 🤷🏻 yard behaves the same.

@deivid-rodriguez
Copy link
Member

Well, you're totally right after a second look. I'm going to merge this and create a feature request to RuboCop.

@deivid-rodriguez
Copy link
Member

Actually, given that the comment could perfectly apply to the require statement, I don't think this can be easily checked through RuboCop without false positives, so we should probably implement some of your initial suggestions.

@nevans
Copy link
Contributor Author

nevans commented Jan 24, 2022

rubocop worked fine on a quick test for me, or at least: it worked the same way as rdoc and yard work. :)

If it isn't desirable on all classes, there's always .rubocop_todo.yml or rubocop:disable or Style/Documentation: { Include: lib/rubygems/version.rb }

Sorry I haven't yet checked out how many exceptions (whether Include or Exclude) would be needed on this repo. If you want, I can try that out (but probably not today).

@deivid-rodriguez
Copy link
Member

You're right again 🤦. Style/Documentation: { Include: lib/rubygems/version.rb } seems fine to me!

@deivid-rodriguez
Copy link
Member

Unfortunately Style/Documentation does not seem to allow this if there's a blank line between the comment and the class opening. I'm just going to merge this since I don't want to block it on such a tiny thing.

@deivid-rodriguez deivid-rodriguez changed the title Fix missing rdoc for Gem::Version Fix missing rdoc for Gem::Version Feb 7, 2022
@deivid-rodriguez deivid-rodriguez merged commit a9e3054 into rubygems:master Feb 7, 2022
@nevans nevans deleted the gem-version-rdoc branch February 7, 2022 14:45
@nevans
Copy link
Contributor Author

nevans commented Feb 7, 2022

Unfortunately Style/Documentation does not seem to allow this if there's a blank line between the comment and the class opening. I'm just going to merge this since I don't want to block it on such a tiny thing.

Ah right. Yeah, even though I personally prefer the rubocop style (no blank lines between comment and target) I wouldn't want to change the project style just to make rubocop happy! I'm guessing this style's been used consistently in this project since probably 2008? Is it seattlerb style? And Eric Hodel was the primary maintainer for rdoc during that time too, so I think maybe he ought to have a say on what is allowed within "proper" rdoc syntax!

One of my 2022 New Years resolutions was to learn how to create and update rubocop rules and autocorrections. IMO, rubocop should be able accommodate styles like this that are already long established in ruby's most significant projects. And adding a config option for this seems like a much simpler starter project than e.g. all of the alignment updates I have in mind. 😉 No promises, but I'll try not to forget this ticket and maybe I'll get back to you in a few weeks or a few months. 🙂

@nevans
Copy link
Contributor Author

nevans commented Feb 7, 2022

Oh, and thanks! 🎉

@deivid-rodriguez
Copy link
Member

Ah right. Yeah, even though I personally prefer the rubocop style (no blank lines between comment and target) I wouldn't want to change the project style just to make rubocop happy! I'm guessing this style's been used consistently in this project since probably 2008? Is it seattlerb style? And Eric Hodel was the primary maintainer for rdoc during that time too, so I think maybe he ought to have a say on what is allowed within "proper" rdoc syntax!

Yes, all of that seems correct to me :)

One of my 2022 New Years resolutions was to learn how to create and update rubocop rules and autocorrections. IMO, rubocop should be able accommodate styles like this that are already long established in ruby's most significant projects. And adding a config option for this seems like a much simpler starter project than e.g. all of the alignment updates I have in mind. 😉 No promises, but I'll try not to forget this ticket and maybe I'll get back to you in a few weeks or a few months. 🙂

That'd be great :)

Oh, and thanks! 🎉

Thanks to you! ❤️

deivid-rodriguez added a commit that referenced this pull request Feb 9, 2022
Fix missing rdoc for Gem::Version

(cherry picked from commit a9e3054)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants