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

Tell SimpleCov to ignore unreachable code #2034

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Conversation

bquorning
Copy link
Collaborator

@bquorning bquorning commented Feb 9, 2025

In some cases we know that a case statement is covering all possible branches, e.g. case [foo?, bar?] has four branches (if the predicate methods return booleans, of course), and case style has a know number of branches, depending on SupportedStyles in config/default.yml.

So, when to use else ... raise ArgumentError, and when to change the last elsif or when into an else? I don't know.

To avoid conflicts with other PRs improving code coverage, the .simplecov file is not updated here.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

@bquorning bquorning requested a review from a team as a code owner February 9, 2025 13:02
@bquorning
Copy link
Collaborator Author

cc @corsonknowles

@bquorning bquorning added this to the Code coverage milestone Feb 9, 2025
@corsonknowles
Copy link
Contributor

I like the use of else blocks!

I agree it's a good sign to be deliberate that it's unreachable code.

At this point, I think we should probably add a contributor guideline to the README so folks know how to handle unreachable code.

Some ways of using node patterns and then further matching in Ruby methods and case handling for config both lend themselves to that outcome, so it will likely come up.

@corsonknowles
Copy link
Contributor

Also FYI, I tried in blocks instead of when blocks for exhaustive pattern matching, and although they work great, they do not change the coverage issue:
https://docs.ruby-lang.org/en/3.0/syntax/pattern_matching_rdoc.html

@bquorning
Copy link
Collaborator Author

I like the use of else blocks!

else is easy, but e.g. when :be_falsey, :be_falsy, :a_falsey_value, :a_falsy_value or when [false, false] is more informative, almost serving as a kind of documentation. Which is why in some places I kept the list of possible values as a comment, and in other places chose to add an else branch that just raises an exception.

@bquorning
Copy link
Collaborator Author

If I remember correctly, case/in blocks must have an else branch. Or is it just that it raises an exception if none of the branches are matched?

@corsonknowles
Copy link
Contributor

corsonknowles commented Feb 9, 2025 via email

@bquorning bquorning force-pushed the improve-code-coverage branch from d9a0658 to 2be7260 Compare February 9, 2025 19:52
@bquorning bquorning requested a review from pirj February 9, 2025 21:18
@bquorning bquorning force-pushed the improve-code-coverage branch from 2be7260 to ca144e2 Compare February 10, 2025 10:27
In some cases we know that a case statement is covering all possible
branches, e.g. `case [foo?, bar?]` has four branches (if the predicate
methods return booleans, of course), and `case style` has a know number
of branches, depending on `SupportedStyles` in config/default.yml.

So, when to use `else ... raise ArgumentError`, and when to change the
last `elsif` or `when` into an `else`? I don't know.
@bquorning bquorning force-pushed the improve-code-coverage branch from ca144e2 to 3870bd0 Compare February 10, 2025 11:30
@@ -2,7 +2,7 @@

SimpleCov.start do
enable_coverage :branch
minimum_coverage line: 100, branch: 98.44
minimum_coverage line: 100, branch: 100
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@corsonknowles Yay ✨

Copy link
Member

Choose a reason for hiding this comment

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

🤩👏

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ :shipit: 🥇

@bquorning
Copy link
Collaborator Author

@pirj 👍🏼❓

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you! 🤩

@bquorning bquorning merged commit e40c361 into master Feb 10, 2025
27 checks passed
@bquorning bquorning deleted the improve-code-coverage branch February 10, 2025 14:43
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.

4 participants