Address PR #909 review comments for YARD documentation #910
Conversation
- Fix typo in runner.rb log message ('will soft shutdown down' -> 'will soft shutdown')
- Fix @example tags in logging formatters to use proper YARD syntax
- Remove @!attribute directives, use standard YARD @return comments for attr_accessor
- Add @raise tag to exponential_backoff_retry.rb documenting exception re-raising
- Fix @return type in active_record middleware (void -> Object for yield returns)
- Remove obsolete ruby2_keywords call (Ruby 3.2+ minimum)
- Add super() call in concurrent_send_adapter.rb for Rails 7.2+ compatibility
- Update cattrs type annotation to clarify key/value semantics
WalkthroughThis pull request makes minor code adjustments and updates documentation across multiple files. The code changes include removing a Ruby keyword-args compatibility hook, conditionally calling a superclass initializer, and updating a log message. The bulk of the changes are documentation improvements to YARD comments and RDoc annotations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/active_job/extensions.rb(1 hunks)lib/active_job/queue_adapters/shoryuken_concurrent_send_adapter.rb(1 hunks)lib/shoryuken/active_job/current_attributes.rb(1 hunks)lib/shoryuken/logging/pretty.rb(1 hunks)lib/shoryuken/logging/without_timestamp.rb(1 hunks)lib/shoryuken/middleware/chain.rb(1 hunks)lib/shoryuken/middleware/server/active_record.rb(1 hunks)lib/shoryuken/middleware/server/exponential_backoff_retry.rb(1 hunks)lib/shoryuken/options.rb(1 hunks)lib/shoryuken/runner.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/shoryuken/runner.rb (3)
lib/shoryuken/options.rb (1)
logger(162-164)lib/shoryuken/logging.rb (2)
logger(39-41)logger(47-49)lib/shoryuken/util.rb (1)
logger(10-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Integrations (3.4)
- GitHub Check: Integrations (3.2)
- GitHub Check: Integrations (3.3)
- GitHub Check: Integrations (3.3)
- GitHub Check: Integrations (3.4)
- GitHub Check: Integrations (3.2)
🔇 Additional comments (11)
lib/active_job/queue_adapters/shoryuken_concurrent_send_adapter.rb (1)
26-30: super() call appropriately respects parent adapter initializationAdding
super() if defined?(super)before setting the handlers ensures any base adapter (e.g., future AbstractAdapter logic) can run its own initialization without changing this adapter’s public API. Looks good.lib/shoryuken/active_job/current_attributes.rb (1)
48-51: Updated @return doc forcattrsmatches actual structureDescribing
cattrsasHash{String => String}of serialization keys to CurrentAttributes class names is accurate and clearer than the previous wording. No further changes needed.lib/shoryuken/logging/without_timestamp.rb (1)
8-12: YARD example formatting is clearer and saferRenaming to “Sample output” and commenting the output line makes the example unambiguous in YARD while matching the actual formatter behavior. Looks good.
lib/shoryuken/middleware/server/exponential_backoff_retry.rb (1)
12-22: @raise documentation correctly reflects call’s failure behaviorDocumenting that
callre-raises the originalStandardErrorwhen retry intervals are missing or exhausted matches the implementation and is useful for middleware authors. No code changes required.lib/shoryuken/logging/pretty.rb (1)
8-12: Consistent, well‑formatted sample output exampleThe updated
@example Sample outputplus commented log line matches the formatter’s actual output and aligns with the style used inWithoutTimestamp. Looks good.lib/shoryuken/middleware/chain.rb (1)
198-205: final_action parameter docs remain accurate after wording changeThe updated
@param final_action [Proc]description still correctly reflects thatinvokeexpects a final action block and calls it after the middleware chain is exhausted. No behavioral impact.lib/shoryuken/runner.rb (1)
130-138: USR1 soft shutdown log typo fixed without changing behaviorThe message now reads cleanly (“will soft shutdown”) while the shutdown path (
@launcher.stopthenexit 0) is unchanged. All good.lib/active_job/extensions.rb (2)
11-14: YARD @return forsqs_send_message_parametersmatches actual usageDocumenting the accessor as
@return [Hash] the SQS send message parametersis accurate and helps clarify what callers can expect to read/mutate.
20-27: ruby2_keywords removal aroundinitializelooks safe for Ruby 3.x, but confirm version floor
initialize(*arguments); super(*arguments); self.sqs_send_message_parameters = {}preserves argument forwarding semantics on modern Ruby without relying onruby2_keywords. This is appropriate as long as the gem’s supported Ruby versions are 3.x-only (as noted in the PR description); please double‑check the gemspec and CI matrix are aligned so Ruby 2.7 isn’t unexpectedly still supported.lib/shoryuken/middleware/server/active_record.rb (1)
12-14: LGTM! Documentation accurately reflects the return behavior.The changes correctly document that:
- The middleware receives an array of objects as arguments
- The method returns the value yielded by the next middleware/worker in the chain (not void)
lib/shoryuken/options.rb (1)
47-48: LGTM!The single
@returntag for a singleattr_readerfollows standard YARD documentation practice.
| # @return [Boolean] whether to enable ActiveJob queue name prefixing | ||
| # @return [Boolean] whether to cache SQS visibility timeout | ||
| # @return [Hash{String => Hash}] the configured processing groups | ||
| # @return [Object] the executor used to launch workers | ||
| # @return [Proc] the code reloader proc for development environments | ||
| # @return [Boolean] whether code reloading is enabled | ||
| # @return [Proc, nil] callback to execute when server starts | ||
| # @return [Proc, nil] callback to execute when server stops | ||
| # @return [Class] the executor class for running workers | ||
| # @return [Shoryuken::WorkerRegistry] the registry for worker classes | ||
| # @return [Array<#call>] handlers for processing exceptions | ||
| attr_accessor :active_job_queue_name_prefixing, :cache_visibility_timeout, | ||
| :groups, :launcher_executor, :reloader, :enable_reloading, | ||
| :start_callback, :stop_callback, :worker_executor, :worker_registry, | ||
| :exception_handlers |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/shoryuken/options.rb | sed -n '20,50p'Repository: ruby-shoryuken/shoryuken
Length of output: 1766
🏁 Script executed:
# Check if YARD is available and what version
gem list | grep -i yard || echo "YARD not found via gem list"
which yard || echo "YARD not in PATH"Repository: ruby-shoryuken/shoryuken
Length of output: 173
🌐 Web query:
YARD documentation multiple @return tags attribute methods how to document attributes
💡 Result:
Short answer — use @!attribute for attributes (with an indented docstring and a single @return describing the type/value), put attribute docs directly above attr_* (or use @!attribute when attributes are created dynamically), and use multiple @return tags only for distinct return cases (each starting with "if ...").
Examples and rules
-
Document a normal attr_accessor/reader/writer by placing the doc above the declaration and a single @return for the getter type. Example:
@return [String] the name
attr_accessor :name
[1][2] -
For dynamically defined attributes (DSL/meta-programming) use the @!attribute directive. You can specify [r], [w], or [rw] and include an indented docstring with @return:
@!attribute [rw] publisher
@return [String] publisher name
cattr_accessor :publisher
[2][3] -
If getter and setter need different docs, keep docs on the reader/declaration and use @overload to describe separate behavior/signatures (or use @!method/@!attribute to create explicit docs). By default YARD exposes the reader. [1][2]
-
Multiple @return tags: allowed only when the method has distinct return cases — each @return should document a specific case and begin with "if …" (e.g. "@return [TypeA] if condition; @return [TypeB] if other condition"). Otherwise list multiple types in one @return (e.g. @return [String, nil]). [4][1]
References
[1] YARD Getting Started — Documenting Attributes (rubydoc.info).
[2] YARD Tags / @!attribute directive (rubydoc.info).
[3] Example usage / community guidance (Stack Overflow).
[4] YARD @return tag documentation and multiple-return-case guidance (rubydoc.info / YARD source).
Refactor documentation to use one @return per attribute method.
This pattern places 11 separate @return tags before a single attr_accessor declaration with 11 comma-separated attributes. YARD documentation conventions specify that each attribute method should have its own documentation block with a single @return tag describing the getter's return type. Multiple @return tags are only appropriate when documenting distinct conditional return cases within the same method, each prefixed with "if ...".
The current structure does not follow YARD best practices and may result in incorrect or missing documentation in the generated YARD output. Each attribute should either:
- Be documented individually above its own declaration, or
- Use the
@!attribute [rw]directive with indented docstrings (useful for DSL-based attributes)
Apply similar corrections to lines 42–45 (attr_writer with 3 @return tags) and verify line 47–48 (attr_reader with 1 @return tag, which is correct).
Summary
This PR addresses the review comments from PR #909 (yard-lint documentation).
Changes made:
'will soft shutdown down'→'will soft shutdown'@returncomments forattr_accessordeclarations instead of@!attributedirectivesvoidtoObjectin active_record middleware since it returns the yielded valueruby2_keywords(:initialize)call (Ruby 3.2+ minimum now)super() if defined?(super)in concurrent_send_adapter.rb for Rails 7.2+ compatibilityTest plan
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.