-
Couldn't load subscription status.
- Fork 445
Simplify switch statement in parse_statements.
#1258
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?
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 tkThey 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
endThere 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_embdocare handled by a singlewhenbranch. In this branch there are multipleifbranches handling:on_nland:on_ignored_nlseparately from:on_commentand:on_embdoc.By having separate
whenbranches for:on_nl/:on_ignored_nland:on_comment/:on_embdocwe can remove theifstatements.