-
Notifications
You must be signed in to change notification settings - Fork 440
feat(yard): add initial support of YARD tags #1416
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
base: master
Are you sure you want to change the base?
feat(yard): add initial support of YARD tags #1416
Conversation
# Extract parameter names from YARD type specification | ||
# e.g., "[String, Integer]" -> "value1, value2" | ||
# e.g., "[item, index]" -> "item, index" | ||
def extract_param_names(params_string) |
There was a problem hiding this comment.
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,
At this time, `@yield`, `@private` and `@api private` are supported.
1ca2295
to
8bc0794
Compare
## | ||
# Extract parameter names from YARD type specification | ||
# e.g., "[String, Integer]" -> "value1, value2" | ||
# e.g., "[item, index]" -> "item, index" |
There was a problem hiding this comment.
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"
return if code_object.block_params && !code_object.block_params.empty? | ||
|
||
# Match @yield [params] description | ||
if text =~ /^\s*#?\s*@yield\s*(?:\[([^\]]*)\])?\s*(.*?)$/ |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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 = ...
meth, | ||
line_no: start_line, | ||
visibility: visibility, | ||
visibility: visibility, # Use YARD visibility if set, otherwise use Ruby visibility |
There was a problem hiding this comment.
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.
|
||
## | ||
# Process @api tags (specifically @api private) | ||
def process_api_tags |
There was a problem hiding this comment.
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 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) |
There was a problem hiding this comment.
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.
|
||
## | ||
# Process comment as YARD comment | ||
def comment=(comment) |
There was a problem hiding this comment.
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.
# 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 |
There was a problem hiding this comment.
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?
# - @private - Marks method/class as private | ||
# - @api private - Alternative private marking | ||
|
||
class RDoc::YARD |
There was a problem hiding this comment.
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.
new(comment, code_object).process | ||
end | ||
|
||
attr_reader :comment, :code_object, :text |
There was a problem hiding this comment.
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 self.process(comment, code_object) | ||
new(comment, code_object).process | ||
end |
There was a problem hiding this comment.
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?
return comment unless comment.is_a?(RDoc::Comment) | ||
|
||
# Skip processing if comment is in TomDoc format | ||
return comment if comment.format == 'tomdoc' |
There was a problem hiding this comment.
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.
return if code_object.block_params && !code_object.block_params.empty? | ||
|
||
# Match @yield [params] description | ||
if text =~ /^\s*#?\s*@yield\s*(?:\[([^\]]*)\])?\s*(.*?)$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yield
can use multiple lines: https://github.com/lsegal/yard/blob/5b93b3a4f15a52f3165edef112e718149d294c38/lib/yard/parser/source_parser.rb#L223-L224
def process_private_tags | ||
return unless code_object.respond_to?(:visibility=) | ||
|
||
if text =~ /^\s*#?\s*@private\s*$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ?
needed after #
?
# e.g., "[String, Integer]" -> "value1, value2" | ||
# e.g., "[item, index]" -> "item, index" | ||
def extract_param_names(params_string) | ||
return '' if params_string.nil? || params_string.empty? |
There was a problem hiding this comment.
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.
return '' if params_string.nil? || params_string.empty? | ||
|
||
# If params look like variable names (lowercase start), use them | ||
if params_string =~ /^[a-z_]/ |
There was a problem hiding this comment.
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)
|
||
# If params look like variable names (lowercase start), use them | ||
if params_string =~ /^[a-z_]/ | ||
params_string.split(',').map(&:strip).join(', ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params_string.split(',').map(&:strip).join(', ') | |
params_string.split(/\s*,\s*/).join(', ') |
At this time,
@yield
,@private
and@api private
are supported.Ref: #1344