Document the rest of the code #118

Mr0grog · 2020-09-01T07:25:26Z

This is meant to merge into #111 rather than the main branch.

This brings GoodJob to 89% documented — the remaining bits are the methods on LogSubscriber, which didn't seem worth documenting since they merely mirror the names of notifications they respond to, so I feel like they're pretty self documenting. These can probably be handled with one of:

:nodoc: comments (which unfortunately show up in the resulting docs).
A macro that makes a pretty simplistic comment.
@api nodoc comments combined with --hide-api nodoc in the .yardopts file. (This will hide them completely.)

Additionally, I didn’t really want to mess with it too much since it looks like some of the methods are for non-existent notifications. Not sure if things have changed since you last touched it or if you were anticipating notifications that haven’t yet been created. These are all the ones that I could see GoodJob sending:

scheduler_shutdown_start
scheduler_shutdown
scheduler_restart_pools
finished_timer_task
finished_job_task
scheduler_create_pools
cleanup_preserved_jobs
enqueue_job
perform_job
notifier_listen
notifier_notified
notifier_notify_error
notifier_unlisten

Don’t know if you want to document them somewhere or not.

This also adds the yard-activesupport-concern gem, which is pretty helpful for documenting methods inside included and class_methods blocks, which are Rails features that are wind up hiding their contents from Yard.

I spent an unfortunate amount of time attempting to make macros work nicely for ActiveRecord’s scope, but never found a solution that didn’t cause strange warnings from Yard, odd behavior with duplicated methods every time you reloaded the docs, or other problems, so I wound up with pretty verbose syntax for those. There might be a nice Yard plugin that solves this better. ¯\_(ツ)_/¯

Finally, I changed the CLI description setup slightly. It turns out we were putting long descriptions where the short, summary descriptions were meant to go, leaving us with this:

$ bundle exec good_job help
Commands:
  good_job cleanup_preserved_jobs  # Deletes preserved job records. By default, GoodJob deletes job records when the job is performed and this command is not necessary. However, ...
  good_job help [COMMAND]          # Describe available commands or one specific command
  good_job start                   # Executes queued jobs. All options can be configured with environment variables. See option descriptions for the matching environment variable...

Now we have the more readable:

$ bundle exec good_job help
Commands:
  good_job cleanup_preserved_jobs  # Deletes preserved job records.
  good_job help [COMMAND]          # Describe available commands or one specific command
  good_job start                   # Executes queued jobs.

As a bonus, it let me make a nice little macro for the doc comments. 😉

This brings GoodJob to 89% documented -- the remaining bits are the methods on `LogSubscriber`, which didn't seem worth documenting since they merely mirror the names of notifications they respond to, so I feel like they're pretty self documenting. These can probably be handled with one of: - `:nodoc:` comments (which unfortunately show up in the resulting docs) - A macro that makes a pretty simplistic comment - `@api nodoc` comments combined with `--hide-api nodoc` in the `.yardopts` file. This will hide them completely. This also adds the `yard-activesupport-concern` gem, which is pretty helpful for documenting methods inside `included` and `class_methods` blocks, which are Rails features that are wind up hiding their contents from Yard.

Mr0grog · 2020-09-01T07:28:40Z

lib/good_job/job.rb

+ # Finds the next eligible Job, acquire an advisory lock related to it, and
+ # executes the job.
+ # @return [Array<GoodJob::Job, Object, Exception>, nil]
+ # If a job was executed, returns an array with the {Job} record, the
+ # return value for the job's +#perform+ method, and the exception the job
+ # raised, if any (if the job raised, then the second array entry will be
+ # +nil+). If there were no jobs to execute, returns +nil+.
 def self.perform_with_advisory_lock


In the README and in other spots, we used the language of “executing” jobs. This made me wonder if we should have said “perform” everywhere instead. Especially since it is the ActiveJob language.

I agree. "Perform" is the correct verb in the context of "jobs".

Unfortunately there are many things that are executed that may or may not result in a job being performed. I'm ambivalent to a search-and-replace, but a documented decision would be helpful.

bensheldon

This is great! Nothing critical needs to be changed but I left some comments for improvement.

I agree about leaving LogSubscriber undocumented.

And I understand about :nodoc: not working, that seems to be a hill YARD wants to die on.

lib/good_job/job.rb

bensheldon · 2020-09-01T15:51:53Z

lib/good_job/job.rb

+ # Finds the next eligible Job, acquire an advisory lock related to it, and
+ # executes the job.
+ # @return [Array<GoodJob::Job, Object, Exception>, nil]
+ # If a job was executed, returns an array with the {Job} record, the
+ # return value for the job's +#perform+ method, and the exception the job
+ # raised, if any (if the job raised, then the second array entry will be
+ # +nil+). If there were no jobs to execute, returns +nil+.
 def self.perform_with_advisory_lock


I agree. "Perform" is the correct verb in the context of "jobs".

Unfortunately there are many things that are executed that may or may not result in a job being performed. I'm ambivalent to a search-and-replace, but a documented decision would be helpful.

lib/good_job/job.rb

lib/good_job/lockable.rb

bensheldon · 2020-09-01T16:07:55Z

Oh, of your options for LogSubscriber, I think the macro one is best for now.

Mr0grog · 2020-09-01T17:50:16Z

of your options for LogSubscriber, I think the macro one is best for now.

Can do. What about the mismatch between method names and the actual notifications? e.g. create, timer_task_finished, and job_finished (just looking at the first three) don’t correspond to actual notifications, or are named similarly but not the same as actual notifications (e.g. timer_task_finished vs. finished_timer_task)?

bensheldon · 2020-09-01T18:06:33Z

I think they have to match or they're broken. So that might just need a todo or fixit.

Mr0grog · 2020-09-01T20:33:55Z

FYI, I’m rearranging some of the methods on LogSubscriber due to this bug in Yard: lsegal/yard#1350

Explain how it mainly relates to transactions and the visibility of new records.

Mr0grog · 2020-09-01T20:49:46Z

I’m going to hold off on “perform” vs. “execute” in this PR since it touches a lot more files than are here, and requires some more nuance.

Ready for re-review.

bensheldon

👍 thank you!

Mr0grog commented Sep 1, 2020

View reviewed changes

bensheldon approved these changes Sep 1, 2020

View reviewed changes

Mr0grog added 2 commits September 1, 2020 10:54

Improve type annotations

ec04091

Document LogSubscriber methods with macro

835d277

Mr0grog added 2 commits September 1, 2020 11:20

Add grouping for notification responders

f418a54

Add FIXME comments for misnamed notifications

d87580c

Mr0grog added 2 commits September 1, 2020 13:37

Fix lint errors in log_subscriber.rb

a0f0245

Clarify create_with_advisory_lock usage

3cf198a

Explain how it mainly relates to transactions and the visibility of new records.

Mr0grog requested a review from bensheldon September 1, 2020 20:49

bensheldon approved these changes Sep 2, 2020

View reviewed changes

bensheldon merged commit 79b78db into bensheldon:code_docs Sep 2, 2020

bensheldon mentioned this pull request Sep 2, 2020

Host a documentation sprint #48

Closed

bensheldon added the documentation Improvements or additions to documentation label Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document the rest of the code #118

Document the rest of the code #118

Mr0grog commented Sep 1, 2020 •

edited

Loading

Mr0grog Sep 1, 2020

bensheldon Sep 1, 2020

bensheldon left a comment

bensheldon Sep 1, 2020

bensheldon commented Sep 1, 2020

Mr0grog commented Sep 1, 2020

bensheldon commented Sep 1, 2020

Mr0grog commented Sep 1, 2020

Mr0grog commented Sep 1, 2020

bensheldon left a comment

Document the rest of the code #118

Document the rest of the code #118

Conversation

Mr0grog commented Sep 1, 2020 • edited Loading

Mr0grog Sep 1, 2020

Choose a reason for hiding this comment

bensheldon Sep 1, 2020

Choose a reason for hiding this comment

bensheldon left a comment

Choose a reason for hiding this comment

bensheldon Sep 1, 2020

Choose a reason for hiding this comment

bensheldon commented Sep 1, 2020

Mr0grog commented Sep 1, 2020

bensheldon commented Sep 1, 2020

Mr0grog commented Sep 1, 2020

Mr0grog commented Sep 1, 2020

bensheldon left a comment

Choose a reason for hiding this comment

Mr0grog commented Sep 1, 2020 •

edited

Loading