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

Fixes bug in Metrics/MethodLength cop. The cop does not count block comments even though count comments setting is turned off. #12658

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KarlHeitmann
Copy link

@KarlHeitmann KarlHeitmann commented Jan 29, 2024

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.

The other day I was trying to reduce the number of lines in the methods of my project. I am not a big fan of block comments but I usually use them to toggle code snippets I want to test later on. I discovered my rubocop LSP instance was complaining that the method had a lot of lines, that was because I commented with =begin ... =end a lot of lines.

The code changes on this PR attempts to fix the issue, so it will not count block comments. The main problem is this "bad API" method was used by CodeLengthCalculator#irrelevant_line?. So, block comments were not considered by the CodeLengthCalculator (because the regex in the "bad API" method does not catch multiline comments (ie, block comments)).

My first approach to solve the issue consisted in using @processed_source to analyze its tokens array. But I found out that the tokens array value is set using tokenise(create_parser(ruby_version)). create_parser must use a specific ruby version class depending on the ruby version used. After realizing that, I thought "Mmmhh... maybe this create_parser method is very expensive in terms of computation and maybe I should not use it..."

Then, I decided to follow a simpler approach: find all the places irrelevant_line? method is called, and add a filter stage just before the method is called. The filter stage consists in calling a recursive method that will remove all lines between a "=begin" tag, and "=end" tag.

For example, by using this CodeLengthCalculator#clean_block_comments recursive method, you will transform this array:

["a = 1\n", "  # This is a comment\n", "=begin\n", "kkkkkk = 92\n", "=end\n", "  # ANOTHER comment\n", "=begin\n", "a = 6\n", "=end\n", "  b = 2"]

and convert that array into this one:

["a = 1\n", "  # This is a comment\n", "  # ANOTHER comment\n", "  b = 2"]

My solution seems to work, I can add more tests to it if you want more. But I'd love to make a more efficient solution to this bug. Any feedback is welcome.

Notes on the checklist item:

  • I don't know if is worth to squash at least the 2 first commits of this PR. If you'd like me to squash the first 2 or all the commits into one, just ping me here.
  • I've introduced some alerts: Metrics/PerceivedComplexity and Metrics/AbcSize. I don't know how to solve these two issues.

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@KarlHeitmann KarlHeitmann changed the title Fixes bug in Metrics/MethodLength cop. The cop does not count block comments even though count comments setting is turned on. Fixes bug in Metrics/MethodLength cop. The cop does not count block comments even though count comments setting is turned off. Jan 29, 2024
@KarlHeitmann
Copy link
Author

I completed the items on the checklist. I don't know if I need to squash more commits, and I don't know either how to fix de rubocop metrics related to high complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant