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

Document public hooks in AS::Reloader #30100

Merged
merged 1 commit into from Nov 17, 2017
Merged

Document public hooks in AS::Reloader #30100

merged 1 commit into from Nov 17, 2017

Conversation

kirs
Copy link
Member

@kirs kirs commented Aug 6, 2017

This PR moves documentation for AS::Reloader hooks outside of hidden rdoc block.
I think it's important to at least document #to_prepare, as it can be quite useful.

@rafaelfranca

@rails-bot
Copy link

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

class Reloader < ExecutionWrapper
define_callbacks :prepare

define_callbacks :class_unload

# A hook run once at application startup and every time the code is reloaded.
# Can be used to monkey patch reloaded constants.
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to keep this second line out. It can be useful for a lot of things, so better to no specify.

@rafaelfranca
Copy link
Member

Great idea!

@matthewd
Copy link
Member

matthewd commented Aug 9, 2017

I think these should be documented as methods, not as hooks -- so, phrased in terms of what the method will do when called ("registers a block .." or something)... or at least as a "callback", which is both literally what they are, and how they're referred to elsewhere (see e.g. after_action, after_commit, after_validation for some variety, but also some consistency).

Let's keep the comprehensive enumeration where it is, too, though. (It's hidden because they're described in terms of their implementation, rather than their app-facing purpose. I agree we need app-facing documentation, but I think the internal one is useful too.)

@kirs
Copy link
Member Author

kirs commented Aug 11, 2017

Thanks for feedback! I updated the PR.

@rafaelfranca rafaelfranca assigned matthewd and unassigned pixeltrix Aug 11, 2017
@matthewd
Copy link
Member

It's a "callback", not a "hook".

@kirs
Copy link
Member Author

kirs commented Aug 14, 2017

Thanks, fixed 👌

@matthewd matthewd merged commit c39ed43 into rails:master Nov 17, 2017
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

6 participants