-
Notifications
You must be signed in to change notification settings - Fork 441
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
base: master
Are you sure you want to change the base?
Conversation
6db1f5c
to
0781905
Compare
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 |
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'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.
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.
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.
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 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
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've added a regression test just to make sure.
0781905
to
e6b36cb
Compare
`:on_nl`, `:on_ignored_nl`, `:on_comment`, `:on_embdoc` are handled by a single `when` branch. In this branch there are multiple `if` branches handling `:on_nl` and `:on_ignored_nl` separately from `:on_comment` and `:on_embdoc`. By having separate `when` branches for `:on_nl`/`:on_ignored_nl` and `:on_comment`/`:on_embdoc` we can remove the `if` statements.
e6b36cb
to
1366f7e
Compare
:on_nl
,:on_ignored_nl
,:on_comment
,:on_embdoc
are handled by a singlewhen
branch. In this branch there are multipleif
branches handling:on_nl
and:on_ignored_nl
separately from:on_comment
and:on_embdoc
.By having separate
when
branches for:on_nl
/:on_ignored_nl
and:on_comment
/:on_embdoc
we can remove theif
statements.