Skip to content
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

Simplify switch statement in parse_statements. #1258

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
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
93 changes: 44 additions & 49 deletions lib/rdoc/parser/ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1804,65 +1804,60 @@ def parse_statements(container, single = NORMAL, current_method = nil,
non_comment_seen = true unless (:on_comment == tk[:kind] or :on_embdoc == tk[:kind])

case tk[:kind]
when :on_nl, :on_ignored_nl, :on_comment, :on_embdoc then
if :on_nl == tk[:kind] or :on_ignored_nl == tk[:kind]
skip_tkspace
tk = get_tk
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the before/after behaviours are actually the same to be honest.

In this condition, the tk variable is reassigned so the later if statement is comparing the new tk instead of the current one. And this behaviour doesn't seem to be replicated in the updated version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line would be followed by the following:

unget_tk tk

They cancel each other out, so I removed them.
The next tk would get picked up in the surrounding while loop.

There is some extra processing done at the end of the while loop, but this seems to always get skipped as try_parse_comment is always false and keep_comment is true in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the following test case for TestRDocParserRuby reproduces this behaviour.
It is green on both branches.

  def test_parse_regression
    util_parser <<-CLASS
class Foo
  # ignored comment

  ##
  # :method: ghost
  # This is a ghost method

  # This is a regular method
  def regular
  end
end
    CLASS

    tk = @parser.get_tk

    @parser.parse_class @top_level, RDoc::Parser::Ruby::NORMAL, tk, @comment

    foo = @top_level.classes.first
    assert_equal 'Foo', foo.full_name

    ghost = foo.method_list.first
    assert_equal 'Foo#ghost', ghost.full_name
    assert_equal 'This is a ghost method', ghost.comment.to_s

    regular = foo.method_list.last
    assert_equal 'Foo#regular', regular.full_name
    assert_equal 'This is a regular method', regular.comment.to_s
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a regression test just to make sure.

else
past_tokens = @read.size > 1 ? @read[0..-2] : []
nl_position = 0
past_tokens.reverse.each_with_index do |read_tk, i|
if read_tk =~ /^\n$/ then
nl_position = (past_tokens.size - 1) - i
break
elsif read_tk =~ /^#.*\n$/ then
nl_position = ((past_tokens.size - 1) - i) + 1
break
end
end
comment_only_line = past_tokens[nl_position..-1].all?{ |c| c =~ /^\s+$/ }
unless comment_only_line then
tk = get_tk
when :on_nl, :on_ignored_nl then
skip_tkspace

keep_comment = true
container.current_line_visibility = nil

when :on_comment, :on_embdoc then
past_tokens = @read.size > 1 ? @read[0..-2] : []
nl_position = 0
past_tokens.reverse.each_with_index do |read_tk, i|
if read_tk =~ /^\n$/ then
nl_position = (past_tokens.size - 1) - i
break
elsif read_tk =~ /^#.*\n$/ then
nl_position = ((past_tokens.size - 1) - i) + 1
break
end
end
comment_only_line = past_tokens[nl_position..-1].all?{ |c| c =~ /^\s+$/ }
unless comment_only_line then
tk = get_tk
end

if tk and (:on_comment == tk[:kind] or :on_embdoc == tk[:kind]) then
if non_comment_seen then
# Look for RDoc in a comment about to be thrown away
non_comment_seen = parse_comment container, tk, comment unless
comment.empty?
if non_comment_seen then
# Look for RDoc in a comment about to be thrown away
non_comment_seen = parse_comment container, tk, comment unless
comment.empty?

comment = ''
comment = RDoc::Encoding.change_encoding comment, @encoding if @encoding
end
comment = ''
comment = RDoc::Encoding.change_encoding comment, @encoding if @encoding
end

line_no = nil
while tk and (:on_comment == tk[:kind] or :on_embdoc == tk[:kind]) do
comment_body = retrieve_comment_body(tk)
line_no = tk[:line_no] if comment.empty?
comment += comment_body
comment << "\n" unless comment_body =~ /\n\z/

if comment_body.size > 1 && comment_body =~ /\n\z/ then
skip_tkspace_without_nl # leading spaces
end
tk = get_tk
line_no = nil
while tk and (:on_comment == tk[:kind] or :on_embdoc == tk[:kind]) do
comment_body = retrieve_comment_body(tk)
line_no = tk[:line_no] if comment.empty?
comment += comment_body
comment << "\n" unless comment_body =~ /\n\z/

if comment_body.size > 1 && comment_body =~ /\n\z/ then
skip_tkspace_without_nl # leading spaces
end
tk = get_tk
end

comment = new_comment comment, line_no
comment = new_comment comment, line_no

unless comment.empty? then
look_for_directives_in container, comment
unless comment.empty? then
look_for_directives_in container, comment

if container.done_documenting then
throw :eof if RDoc::TopLevel === container
container.ongoing_visibility = save_visibility
end
if container.done_documenting then
throw :eof if RDoc::TopLevel === container
container.ongoing_visibility = save_visibility
end

keep_comment = true
else
non_comment_seen = true
end

unget_tk tk
Expand Down
48 changes: 48 additions & 0 deletions test/rdoc/test_rdoc_parser_ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,54 @@ class Foo
assert_equal 2, foo.method_list.length
end

def test_parse_comments_followed_by_linebreaks
util_parser <<-CLASS
class Foo
# Ignored comment

##
# :method: ghost
# This is a ghost method

# Comment followed by a method
def regular
end

##
# :method: another_ghost
# This is another ghost method

# Comment followed by a linebreak

def regular2
end
end
CLASS

tk = @parser.get_tk

@parser.parse_class @top_level, RDoc::Parser::Ruby::NORMAL, tk, @comment

foo = @top_level.classes.first
assert_equal 'Foo', foo.full_name

ghost = foo.method_list.first
assert_equal 'Foo#ghost', ghost.full_name
assert_equal 'This is a ghost method', ghost.comment.to_s

regular = foo.method_list[1]
assert_equal 'Foo#regular', regular.full_name
assert_equal 'Comment followed by a method', regular.comment.to_s

another_ghost = foo.method_list[2]
assert_equal 'Foo#another_ghost', another_ghost.full_name
assert_equal 'This is another ghost method', another_ghost.comment.to_s

regular2 = foo.method_list[3]
assert_equal 'Foo#regular2', regular2.full_name
assert_equal 'Comment followed by a linebreak', regular2.comment.to_s
end

def test_parse_class_nodoc
comment = RDoc::Comment.new "##\n# my class\n", @top_level

Expand Down
Loading