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

Add docs for ActiveSupport::Notifications.monotonic_subscribe [ci-skip] #45569

Conversation

whyinzoo
Copy link
Contributor

Summary
Adding docs to ActiveSupport::Notifications.monotonic_subscribe

Found method through https://www.codetriage.com/

Comment on lines 247 to 253
# Subscribe to an event name with the passed +block+.
#
# To subscribe to events, pass a String to match exact event
# names, or pass a Regexp to match all events that match a pattern.
#
# The +block+ will receive five parameters with information about the event:
#
# ActiveSupport::Notifications.monotonic_subscribe('render') do |name, start, finish, id, payload|
# name # => String, name of the event (such as 'render' from above)
# start # => Monotonic time, when the instrumented block started execution
# finish # => Monotonic time, when the instrumented block ended execution
# id # => String, unique ID for the instrumenter that fired the event
# payload # => Hash, the payload
# end
#
# The +start+ and +finish+ values above represent monotonic time.
#
# If the block passed to the method only takes one parameter,
# it will yield an event object to the block:
#
# ActiveSupport::Notifications.monotonic_subscribe(/render/) do |event|
# @event = event
# end
#
# Raises an error if invalid event name type is passed:
#
# ActiveSupport::Notifications.monotonic_subscribe(:render) {|*args| ...}
# #=> ArgumentError (pattern must be specified as a String, Regexp or empty)
#
Copy link
Member

Choose a reason for hiding this comment

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

I see that most of this is copied from ActiveSupport::Notifications.subscribe. Instead of doing that, let's just explain monotonic_subscribe in terms of subscribe, and explain when the user might want to use monotonic_subscribe instead of subscribe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your feedback is greatly appreciated! I'll make the changes.

@jonathanhefner jonathanhefner changed the title Add docs for ActiveSupport::Notifications.monotonic_subscribe Add docs for ActiveSupport::Notifications.monotonic_subscribe [ci-skip] Jul 13, 2022
@whyinzoo whyinzoo force-pushed the whyinzoo-update-docs-ActiveSupport--Notification-monotonic-subscribe-for-pr branch from faac273 to 4fe64f1 Compare July 14, 2022 04:33
Comment on lines 247 to 253
# Performs the same functionality as +subscribe+, but +start+ and +finish+ within +block+
# are in monotonic time.
#
# Monotonic time represents elapsed time and always moves forward.
#
# Use +monotonic_subscribe+ when time duration is important.
#
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be clear as a single paragraph. And I think we can be a little more specific about when it is "important".

Also, a few notes:

  • We can link to methods like subscribe by prepending #.
  • When possible, we try to wrap text at 80 characters.
  • A trailing # line is not necessary. It is sometimes added after an indented code example.
Suggested change
# Performs the same functionality as +subscribe+, but +start+ and +finish+ within +block+
# are in monotonic time.
#
# Monotonic time represents elapsed time and always moves forward.
#
# Use +monotonic_subscribe+ when time duration is important.
#
# Performs the same functionality as #subscribe, but the +start+ and
# +finish+ block arguments are in monotonic time instead of wall-clock
# time. Monotonic time always moves forward (never jumps backward). Use
# +monotonic_subscribe+ when accuracy of time duration is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really appreciate the feedback! I'll keep those notes in mind for future documentations!

@whyinzoo whyinzoo force-pushed the whyinzoo-update-docs-ActiveSupport--Notification-monotonic-subscribe-for-pr branch from 4fe64f1 to b39c0fe Compare July 16, 2022 03:44
@jonathanhefner
Copy link
Member

Thank you, @whyinzoo! 😄

@whyinzoo whyinzoo force-pushed the whyinzoo-update-docs-ActiveSupport--Notification-monotonic-subscribe-for-pr branch from b39c0fe to e3de787 Compare July 21, 2022 03:21
@whyinzoo
Copy link
Contributor Author

@jonathanhefner I've updated the description slightly and a bit more specific on when it's "important". Please let me know if the description works :)

@jonathanhefner jonathanhefner merged commit aced3bb into rails:main Jul 21, 2022
@jonathanhefner
Copy link
Member

jonathanhefner commented Jul 21, 2022

Hey, @whyinzoo! I could have sworn I clicked the merge button for this PR right before I posted #45569 (comment). I'm not sure what happened, but I apologize for the oversight!

In any case, the updates look good. Thank you again! 🙌

(Backported to 7-0-stable.)

jonathanhefner added a commit that referenced this pull request Jul 24, 2022
…pport--Notification-monotonic-subscribe-for-pr

Add docs for ActiveSupport::Notifications.monotonic_subscribe [ci-skip]

(cherry picked from commit aced3bb)
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