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

Include READMEs in main framework pages of the API documentation #47717

Merged
merged 1 commit into from Mar 30, 2023

Conversation

p8
Copy link
Member

@p8 p8 commented Mar 20, 2023

Currently when opening the main framework API pages there is no introduction to the framework. Instead we only see a whole lot of modules and the gem_version and version methods.

By including the READMEs using the :include: directive each frameworks has a nice introduction.
For markdown READMEs we need to add the :markup: directive.

[ci-skip]

Thanks to @zzak for the tip of the :include: directive.

Before

image

image

After

image

image

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@p8
Copy link
Member Author

p8 commented Mar 20, 2023

I've also created a PR for sdoc to move the file links to the bottom.
This would result in the following:
image

Currently when opening the main framework pages there is no introduction
to the framework. Instead we only see a whole lot of modules and the
`gem_version` and `version` methods.

By including the READMEs using the `:include:` directive each frameworks
has a nice introduction.
For markdown READMEs we need to add the :markup: directive.

[ci-skip]

Co-authored-by: zzak <zzakscott@gmail.com>
@zzak
Copy link
Member

zzak commented Mar 22, 2023

It would be nice to get a preview of this on the PR, I'm actively trying to make that happen. 🙏

In the mean time I will have to pull this down and take a look.

Too bad the RDoc autolinks Model from the readme, we will have to escape it (e.g. Active \Model) like we do with RDOC_MAIN.md 😢

@@ -10,13 +10,15 @@ class Task < RDoc::Task
"activesupport" => {
include: %w(
README.rdoc
lib/active_support.rb
Copy link
Member

Choose a reason for hiding this comment

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

By including these there is a side-effect that the methods defined in those files are now considered public API if they are not otherwise :nodoc:'d. That may have been intentional, however. Just pointing it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll :nodoc: those.

Copy link
Member Author

@p8 p8 Mar 30, 2023

Choose a reason for hiding this comment

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

@zzak If any of those methods are considered private we should :nodoc: them.
Other generators like Rdoc Info do include these files already and will consider it public API:
https://www.rubydoc.info/github/rails/rails/ActiveSupport.cache_format_version=

For some methods we already add :nodoc:, so I'm guessing it's not common knowledge some of these methods aren't shown in the API docs :

cattr_accessor :test_order # :nodoc:
cattr_accessor :test_parallelization_threshold, default: 50 # :nodoc:
@error_reporter = ActiveSupport::ErrorReporter.new
singleton_class.attr_accessor :error_reporter # :nodoc:

So I guess this fixes a bug where public API methods weren't shown before.

Copy link
Member

Choose a reason for hiding this comment

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

My really honest guess is that API being visible on the website is secondary to the :nodoc: it might have in the source code in terms of weight it has over whether or not it's intended to be public. This is a fair assumption IMO.

If there is no :nodoc: it should be considered public otherwise. After that it's either find the person with context or can make a decision about the visibility or take a wild guess 😂

Copy link
Member

@zzak zzak left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks @p8!

@p8 p8 merged commit dc0f205 into rails:main Mar 30, 2023
@p8 p8 deleted the docs/include-readmes branch March 30, 2023 14:43
@p8
Copy link
Member Author

p8 commented Mar 30, 2023

Thanks @zzak!

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

2 participants