Skip to content

Remove unnecessary coupling in comments' call_seq extraction #1289

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Remove unnecessary coupling in comments' call_seq extraction #1289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lib/rdoc/code_object/any_method.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def call_seq
# See also #param_seq

def call_seq= call_seq
return if call_seq.empty?
return if call_seq.nil? || call_seq.empty?

@call_seq = call_seq
Comment on lines -113 to 115
Copy link
Member

@kou kou Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's not related to this PR but we may want to set @call_seq even when call_seq is nil or "":

call_seq = nil if call_seq&.empty?
@call_seq = call_seq

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it but it's potentially risky if RDoc relies on this to avoid erasing the current value. Will need to do some testing to make sure it's ok to drop it.

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense.

end
Expand Down
5 changes: 1 addition & 4 deletions lib/rdoc/comment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def == other # :nodoc:
# # ARGF.to_a(limit) -> array
# # ARGF.to_a(sep, limit) -> array

def extract_call_seq method
def extract_call_seq
# we must handle situations like the above followed by an unindented first
# comment. The difficulty is to make sure not to match lines starting
# with ARGF at the same indent, but that are after the first description
Expand All @@ -116,10 +116,7 @@ def extract_call_seq method
@text.slice! all_start...all_stop

seq.gsub!(/^\s*/, '')
method.call_seq = seq
end

method
end

##
Expand Down
2 changes: 1 addition & 1 deletion lib/rdoc/parser/c.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ def find_const_comment(type, const_name, class_name = nil)

def find_modifiers comment, meth_obj
comment.normalize
comment.extract_call_seq meth_obj
meth_obj.call_seq = comment.extract_call_seq

look_for_directives_in meth_obj, comment
end
Expand Down
4 changes: 2 additions & 2 deletions lib/rdoc/parser/prism_ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ def handle_meta_method_comment(comment, node)
meth.singleton = @singleton || singleton_method
handle_consecutive_comment_directive(meth, comment)
comment.normalize
comment.extract_call_seq(meth)
meth.call_seq = comment.extract_call_seq
meth.comment = comment
if node
tokens = visible_tokens_from_location(node.location)
Expand Down Expand Up @@ -520,7 +520,7 @@ def add_method(name, receiver_name:, receiver_fallback_type:, visibility:, singl
handle_consecutive_comment_directive(meth, comment)

comment.normalize
comment.extract_call_seq(meth)
meth.call_seq = comment.extract_call_seq
meth.comment = comment
end
handle_modifier_directive(meth, start_line)
Expand Down
6 changes: 3 additions & 3 deletions lib/rdoc/parser/ruby.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,7 @@ def parse_comment_ghost container, text, name, column, line_no, # :nodoc:
end

comment.normalize
comment.extract_call_seq meth
meth.call_seq = comment.extract_call_seq

return unless meth.name

Expand Down Expand Up @@ -1417,7 +1417,7 @@ def parse_meta_method_params container, single, meth, tk, comment # :nodoc:

look_for_directives_in meth, comment
comment.normalize
comment.extract_call_seq meth
meth.call_seq = comment.extract_call_seq

container.add_method meth

Expand Down Expand Up @@ -1485,7 +1485,7 @@ def parse_method(container, single, tk, comment)
parse_method_params_and_body container, single, meth, added_container

comment.normalize
comment.extract_call_seq meth
meth.call_seq = comment.extract_call_seq

meth.comment = comment

Expand Down
53 changes: 11 additions & 42 deletions test/rdoc/test_rdoc_comment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

require_relative 'helper'

class TestRDocComment < RDoc::TestCase
class RDocCommentTest < RDoc::TestCase

def setup
super
Expand Down Expand Up @@ -42,74 +42,54 @@ def test_equals2
end

def test_extract_call_seq
m = RDoc::AnyMethod.new nil, 'm'

comment = RDoc::Comment.new <<-COMMENT, @top_level
call-seq:
bla => true or false

moar comment
COMMENT

comment.extract_call_seq m

assert_equal "bla => true or false\n", m.call_seq
assert_equal "bla => true or false\n", comment.extract_call_seq
end

def test_extract_call_seq_blank
m = RDoc::AnyMethod.new nil, 'm'

comment = RDoc::Comment.new <<-COMMENT, @top_level
call-seq:
bla => true or false

COMMENT

comment.extract_call_seq m

assert_equal "bla => true or false\n", m.call_seq
assert_equal "bla => true or false\n", comment.extract_call_seq
end

def test_extract_call_seq_commented
m = RDoc::AnyMethod.new nil, 'm'

comment = RDoc::Comment.new <<-COMMENT, @top_level
# call-seq:
# bla => true or false
#
# moar comment
COMMENT

comment.extract_call_seq m

assert_nil m.call_seq
assert_nil comment.extract_call_seq
end

def test_extract_call_seq_no_blank
m = RDoc::AnyMethod.new nil, 'm'

comment = RDoc::Comment.new <<-COMMENT, @top_level
call-seq:
bla => true or false
COMMENT

comment.extract_call_seq m

assert_equal "bla => true or false\n", m.call_seq
assert_equal "bla => true or false\n", comment.extract_call_seq
end

def test_extract_call_seq_undent
m = RDoc::AnyMethod.new nil, 'm'

comment = RDoc::Comment.new <<-COMMENT, @top_level
call-seq:
bla => true or false
moar comment
COMMENT

comment.extract_call_seq m

assert_equal "bla => true or false\nmoar comment\n", m.call_seq
assert_equal "bla => true or false\nmoar comment\n", comment.extract_call_seq
end

def test_extract_call_seq_c
Expand All @@ -128,18 +108,14 @@ def test_extract_call_seq_c
and commercial week day given. Ignores the 4th argument.
COMMENT

method_obj = RDoc::AnyMethod.new nil, 'blah'

comment.extract_call_seq method_obj

expected = <<-CALL_SEQ.chomp
commercial() -> Date <br />
commercial(cwyear, cweek=41, cwday=5, sg=nil) -> Date [ruby 1.8] <br />
commercial(cwyear, cweek=1, cwday=1, sg=nil) -> Date [ruby 1.9]

CALL_SEQ

assert_equal expected, method_obj.call_seq
assert_equal expected, comment.extract_call_seq
end

def test_extract_call_seq_c_no_blank
Expand All @@ -150,18 +126,14 @@ def test_extract_call_seq_c_no_blank
commercial(cwyear, cweek=1, cwday=1, sg=nil) -> Date [ruby 1.9]
COMMENT

method_obj = RDoc::AnyMethod.new nil, 'blah'

comment.extract_call_seq method_obj

expected = <<-CALL_SEQ.chomp
commercial() -> Date <br />
commercial(cwyear, cweek=41, cwday=5, sg=nil) -> Date [ruby 1.8] <br />
commercial(cwyear, cweek=1, cwday=1, sg=nil) -> Date [ruby 1.9]

CALL_SEQ

assert_equal expected, method_obj.call_seq
assert_equal expected, comment.extract_call_seq
end

def test_extract_call_seq_c_separator
Expand All @@ -183,10 +155,6 @@ def test_extract_call_seq_c_separator

COMMENT

method_obj = RDoc::AnyMethod.new nil, 'blah'

comment.extract_call_seq method_obj

expected = <<-CALL_SEQ
ARGF.readlines(sep=$/) -> array
ARGF.readlines(limit) -> array
Expand All @@ -196,7 +164,7 @@ def test_extract_call_seq_c_separator
ARGF.to_a(sep, limit) -> array
CALL_SEQ

assert_equal expected, method_obj.call_seq
assert_equal expected, comment.extract_call_seq

expected = <<-'COMMENT'

Expand All @@ -211,11 +179,12 @@ def test_extract_call_seq_c_separator
assert_equal expected, comment.text
end

# This test relies on AnyMethod#call_seq's behaviour as well
def test_extract_call_linear_performance
pre = ->(n) {[n, RDoc::Comment.new("\n"*n + 'call-seq:' + 'a'*n)]}
method_obj = RDoc::AnyMethod.new nil, 'blah'
assert_linear_performance((2..5).map {|i| 10**i}, pre: pre) do |n, comment|
comment.extract_call_seq method_obj
method_obj.call_seq = comment.extract_call_seq
assert_equal n, method_obj.call_seq.size
end
end
Expand Down