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

Conversation

p8
Copy link
Contributor

@p8 p8 commented Dec 28, 2024

: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.

@p8 p8 force-pushed the refactor/switch-statement branch from 6db1f5c to 0781905 Compare December 28, 2024 16:41
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.

@p8 p8 force-pushed the refactor/switch-statement branch from 0781905 to e6b36cb Compare December 29, 2024 09:40
`: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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants