Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/rdoc/code_object.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
# frozen_string_literal: true

require_relative 'yard'

##
# Base class for the RDoc code tree.
#
Expand Down
14 changes: 14 additions & 0 deletions lib/rdoc/code_object/constant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,20 @@ def store=(store)
@file = @store.add_file @file.full_name if @file
end

##
# Process comment as YARD comment
def comment=(comment)
Copy link
Member

Choose a reason for hiding this comment

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

Handling YARD in comment= seems too aggressive. It can be triggered from many different situation and places.

def add_alias
  # Maybe this is ok, but I can't confidently say that this is safe
  method.comment = an_alias.comment
end

How about explicitly processing it in RDoc::Parser::PrismRuby?

def add_comment
  ...
  comment.normalize
  override_visibility, some_other_information = handle_yard(meth, comment)
  if override_visibility
    do_something
  end
  meth.comment = comment
  ...
end

RDoc::Parser::Ruby is too complicated (and buggy) to maintain, so the system design of YARD-support shouldn't be affected by RDoc::Parser::Ruby implementation detail. I think supporting YARD only in RDoc::Parser::PrismRuby is enough.

# Process YARD tags while we still have the RDoc::Comment object
if comment.is_a?(RDoc::Comment)
RDoc::YARD.process(comment, self)
end

# Then set the comment normally
super(comment)

@comment
end

def to_s # :nodoc:
parent_name = parent ? parent.full_name : '(unknown)'
if is_alias_for
Expand Down
14 changes: 14 additions & 0 deletions lib/rdoc/code_object/method_attr.rb
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,20 @@ def store=(store)
@file = @store.add_file @file.full_name if @file
end

##
# Process comment as YARD comment
def comment=(comment)
# Process YARD tags while we still have the RDoc::Comment object
if comment.is_a?(RDoc::Comment)
RDoc::YARD.process(comment, self)
Copy link
Member

Choose a reason for hiding this comment

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

In the comment below, @foo is not a YARD tag.

# @foo &&= bar
# ^^^^^^^^^^^^
def visit_instance_variable_and_write_node(node)
end

Ruby's comment frequently contains code example that uses instance variable. It's the same format as YARD tags.

So we need to make YARD feature opt-in, or only accept specific yard tags. (opt-in is better)

# This is just an idea. Specify that this file uses YARD format.
# :markup: yard

class A
  # @yard-tag
  def foo; end
end
# found 37 files in github code search
# @api = ...

end

# Then set the comment normally
super(comment)

@comment
end

def find_see # :nodoc:
return nil if singleton || is_alias_for

Expand Down
10 changes: 9 additions & 1 deletion lib/rdoc/parser/prism_ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -510,13 +510,21 @@ def add_method(name, receiver_name:, receiver_fallback_type:, visibility:, singl

receiver = receiver_name ? find_or_create_module_path(receiver_name, receiver_fallback_type) : @container
meth = RDoc::AnyMethod.new(nil, name, singleton: singleton)

if (comment = consecutive_comment(start_line))
handle_consecutive_comment_directive(@container, comment)
handle_consecutive_comment_directive(meth, comment)

comment.normalize
meth.call_seq = comment.extract_call_seq

# Save visibility before comment assignment (where YARD processing happens)
visibility_before = meth.visibility
meth.comment = comment
# Check if YARD tags changed the visibility
if meth.visibility != visibility_before
visibility = meth.visibility
end
Comment on lines +521 to +527
Copy link
Member

Choose a reason for hiding this comment

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

It seems that this shows that the proposed approach isn't integrated with the current implementation clearly. Can we use another more integrated approach?

end
handle_modifier_directive(meth, start_line)
handle_modifier_directive(meth, args_end_line)
Expand All @@ -527,7 +535,7 @@ def add_method(name, receiver_name:, receiver_fallback_type:, visibility:, singl
receiver,
meth,
line_no: start_line,
visibility: visibility,
visibility: visibility, # Use YARD visibility if set, otherwise use Ruby visibility
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be where YARD actually overrides visibility, near visibility = meth.visibility line.

params: params,
calls_super: calls_super,
block_params: block_params,
Expand Down
134 changes: 134 additions & 0 deletions lib/rdoc/yard.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
# frozen_string_literal: true

##
# YARD compatibility module for RDoc
#
# This module provides support for parsing YARD tags in RDoc comments.
# Currently supports:
# - @yield [params] - Describes block parameters
# - @private - Marks method/class as private
# - @api private - Alternative private marking

class RDoc::YARD
Copy link
Member

Choose a reason for hiding this comment

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

Can we improve this name?

I think that "YARD" isn't suitable here because we don't support full YARD features. We only focus on YARD tags here.


##
# Processes YARD tags in the given comment text for a code object
#
# Returns modified comment text with YARD tags processed
def self.process(comment, code_object)
new(comment, code_object).process
end
Comment on lines +18 to +20
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this shortcut?


attr_reader :comment, :code_object, :text
Copy link
Member

Choose a reason for hiding this comment

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

Do we need them?


def initialize(comment, code_object)
@comment = comment
@code_object = code_object
end

def process
return comment unless comment && code_object

# Only process RDoc::Comment objects to avoid modifying plain strings
# that aren't actually documentation comments
return comment unless comment.is_a?(RDoc::Comment)
Comment on lines +30 to +34
Copy link
Member

Choose a reason for hiding this comment

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

These guards are not needed. Method signature will be simple.

(String | RDoc::Comment | nil, RDoc::CodeObject | nil) -> void
# ↓
(RDoc::Comment, RDoc::CodeObject) -> void

RDoc::Parser::PrismRuby don't set String as comment, and it will be the default in the future.


# Skip processing if comment is in TomDoc format
return comment if comment.format == 'tomdoc'
Copy link
Member

Choose a reason for hiding this comment

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

Can we use comment.tomdoc?

BTW, is this a responsibility of this class? It seems that this is a responsibility of callers.


@text = comment.text.dup

# Process @yield tags
process_yield_tags

# Process @private tags
process_private_tags

# Process @api tags
process_api_tags
Comment on lines +41 to +48
Copy link
Member

Choose a reason for hiding this comment

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

In general, we should not parse the target multiple times by "start parsing -> extract a tag -> process the tag -> remove the tag -> finish parsing" one by one destructively (gsub!). It may require additional cares in following processes.
See also: https://magazine.rubyist.net/articles/0010/0010-CodeReview.html#pukipastr_parse (Japanese)

Can we parse the comment only once?


# Only update comment.text if we actually modified it
# This preserves the @document for comments created from parsed documents
if text != comment.text
comment.text = text
end
Comment on lines +50 to +54
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to update comment destructively in this class?

It seems that we can receive comment.text in #initialize and returns (not destructively) processed text. Updating comment.text is a responsibility of the callers.


comment
end

private

##
# Process @yield tags to extract block parameters
def process_yield_tags
return unless code_object.respond_to?(:block_params=)

# Skip if already has block_params from :yields: directive
return if code_object.block_params && !code_object.block_params.empty?

# Match @yield [params] description
if text =~ /^\s*#?\s*@yield\s*(?:\[([^\]]*)\])?\s*(.*?)$/
Copy link
Member

Choose a reason for hiding this comment

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

YARD tag have a common format # @tagname free_format_argument_part # @!tagname free_format_argument_part
First scan tags in a generic way, and pass it to each tag-process part is better.
This way, parsing multiline tags would be easier.

Copy link
Member

Choose a reason for hiding this comment

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

params = $1

if params && !params.empty?
# Clean up the params - remove types if present
clean_params = extract_param_names(params)
code_object.block_params = clean_params unless clean_params.empty?
end

# Remove the @yield line from comment
text.gsub!(/^\s*#?\s*@yield.*?$\n?/, '')
end
end

##
# Process @private tags to set visibility
def process_private_tags
return unless code_object.respond_to?(:visibility=)

if text =~ /^\s*#?\s*@private\s*$/
Copy link
Member

Choose a reason for hiding this comment

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

Why is ? needed after #?

code_object.visibility = :private

# Remove the @private line from comment
text.gsub!(/^\s*#?\s*@private\s*$\n?/, '')
end
end

##
# Process @api tags (specifically @api private)
def process_api_tags
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 @api private sets documenting level visibility which RDoc doesn't have. It's not for making a method private_method.
One idea is to let it act as nodoc for now.

return unless code_object.respond_to?(:visibility=)

if text =~ /^\s*#?\s*@api\s+private\s*$/
code_object.visibility = :private

# Remove the @api private line from comment
text.gsub!(/^\s*#?\s*@api\s+private\s*$\n?/, '')
end
end

##
# Extract parameter names from YARD type specification
# e.g., "[String, Integer]" -> "value1, value2"
# e.g., "[item, index]" -> "item, index"
Copy link
Member

Choose a reason for hiding this comment

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

Example and implementation does not match.
"String, Integer" -> "arg1, arg2"
"item, index" -> "item, index"

def extract_param_names(params_string)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is kind of too much, but YARD's @yield might cause confusion to use type name instead of param name. Here it replaces type name with temporary param name,

return '' if params_string.nil? || params_string.empty?
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these checks?

I feel that it's responsibility of callers.


# If params look like variable names (lowercase start), use them
if params_string =~ /^[a-z_]/
Copy link
Member

Choose a reason for hiding this comment

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

We should use \A not ^ for the start of string. ^ is for the start of line.
See also: https://www.clear-code.com/blog/2015/3/25.html (Japanese)

params_string.split(',').map(&:strip).join(', ')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
params_string.split(',').map(&:strip).join(', ')
params_string.split(/\s*,\s*/).join(', ')

else
# If they look like types (uppercase start), generate generic names
types = params_string.split(',').map(&:strip)
types.each_with_index.map do |type, i|
if type =~ /^[A-Z]/
# It's a type, generate a generic param name
"arg#{i + 1}"
else
# It's already a param name
type
end
end.join(', ')
end
end
end
Loading
Loading