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

[ci skip] Align documentation with conventional practice #51873

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yawboakye
Copy link
Contributor

Motivation / Background

This pull request has been created to (1) align documentation with conventional practice, (2) prefer continuity by maintaining names from previous code blocks, and (3) clear up other potential sources of confusion.

Detail

A search on GitHub reveals that, typically, custom Railtie classes are named Railtie, with Rails itself embracing this convention (perhaps even fostering it). If we named the class in the docs to align with this practice, learners can supplement the material with what they find on GitHub and within Rails itself.

Secondly, the first time we introduce a custom Railtie class, it's called Railtie. Unfortunately, in subsequent code examples, MyRailtie is introduced. In the first code block, we indicate the file path where the class is/could be defined (lib/my_gem/railtie.rb). In the subsequent blocks we don't. I think this could also be a little confusing. If we were to keep the old class name, it might be easy for a learner to infer that we're developing the same class we created in the first code block.

The other changes replace MyRailtie and substitute it with MyGem. Reason is, the custom Railtie doesn't seem to be the appropriate place for those functions given that they are very specific to the gem. I think what we want to encourage there that the Railtie lets you specify what code to run for every hook. Your gem, not the custom Railtie, hold the code. For example, I was very confused by MyRailtie.setup!. I wondered where the setup! method came from and whether it could have been inherited from the Rails::Railtie parent. So I replaced MyRailtie with MyGem to make the arbitrariness of that code more obvious.

Checklist

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why.

and in the process, clear up potential sources of confusion for
learners.
@rails-bot rails-bot bot added the railties label May 21, 2024
@@ -50,18 +50,18 @@ module Rails
# To add an initialization step to the \Rails boot process from your railtie, just
# define the initialization code with the +initializer+ macro:
#
# class MyRailtie < Rails::Railtie
Copy link
Member

@zzak zzak May 21, 2024

Choose a reason for hiding this comment

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

My first thought was keep MyRailtie because it's obvious we're defining a custom class we own, not to be confused with the Railtie class. It maybe a convention that Rails follows for the framework itself, but that doesn't mean every gem has to apply that same naming. And actually that is more confusing to a beginner reading about this from that class with the same name, I can imagine.

On second thought, given the path is my_gem/railtie it's better to name it following Zeitwerk's naming preference and then use autoload. We can also remove the require in the previous example, then?

Need to give this more thought.

There is also a guide for creating plugins, and I wonder if that is not a better place for this type of complete tutorial. I can imagine the guide can give a full in-depth example and reference back to this class for more details, once that is established the naming and assumptions in this API doc can be more consistent.

Since the Rails Foundation is revamping the guides, I have made a note to pass on this information when they get to Plugins.

Also side-note this example seems to be largely unchanged since 781d0a9, fwiw. So different patterns and conventions have sprung up since then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zzak agreed that the explicit require could go away. i'll cut it out while you ponder possible next steps here.

zeitwerk can successfully load the railtie class since it follows
conventions.
@yawboakye
Copy link
Contributor Author

@zzak what are the next steps here? happy to close if in your judgement the modifications don't add much.

@yawboakye yawboakye requested a review from zzak May 23, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants