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

livecheck: refactor livecheck_strategy_names #19351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samford
Copy link
Member

@samford samford commented Feb 22, 2025

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This refactors the livecheck_strategy_names method to align with Doug's livecheck_find_versions_parameters implementation in #19312 (which is currently part of the Options PR).

@samford samford force-pushed the livecheck/refactor-livecheck_strategy_names branch from e41cb33 to 2bca214 Compare February 22, 2025 01:15
This refactors the `livecheck_strategy_names` method to align with
Doug's `livecheck_find_versions_parameters` implementation.
@samford samford force-pushed the livecheck/refactor-livecheck_strategy_names branch from 2bca214 to efeff90 Compare February 22, 2025 01:24
Copy link
Member

@dduugg dduugg left a comment

Choose a reason for hiding this comment

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

lgtm, though it'd be nice if we could cover all of it with tests

@samford
Copy link
Member Author

samford commented Feb 23, 2025

Unless I'm overlooking something, livecheck_strategy_names should be fully covered by the tests. The lower coverage number for the patch comes from the changes in other livecheck/livecheck.rb methods.

I'm ignoring the other untested areas for now, as I'm planning to improve test coverage for livecheck/livecheck.rb after some planned refactoring (I have a few PRs to work through first). I've recently been working on maximizing test coverage in all the other parts of livecheck and some of my upcoming PRs (after Options) will bring test coverage up to 100% with the exception of livecheck/livecheck.rb and maybe livecheck_version.rb (I haven't been able to figure out how to test some parts of LivecheckVersion#<==>, so it's stuck at 92.11% line coverage and 86.67% branch coverage).

Besides that, I've also been working on expanding typed: strong support. livecheck/livecheck.rb is going to take the most work, so I'm also deferring that until after refactoring. I'm not yet sure if typed: strong will be entirely feasible in that file but I'm planning to do as much as possible.

Long story short, the end goal is to have as much covered as possible but it's a work in progress (and not something that I want to dig into in this PR when the other changes are minimal).

@dduugg
Copy link
Member

dduugg commented Feb 23, 2025

Thanks for elaborating. To be clear, I was only referring to the codecov comment.

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.

2 participants