-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: min_coverage parsing with default to 100 #290
fix: min_coverage parsing with default to 100 #290
Conversation
This may be a breaking change. The current behavior is |
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.
LGTM! Thank you for working on this! 💙
@alestiago with the addition of |
I personally don't think this is a breaking change since specifying min_coverage smaller than 0 or greater than 100 was incorrect usage. However, perhaps a less "breaking" solution would be to clamp the values instead of throwing an error, but that might be less intuitive. Either way, I'm fine with both approaches. |
@alestiago can you add the hacktoberfest tag? |
@kelvinwieth I'm not sure how do you want me to do so. Unfortunately this year we didn't prepare to participate in Hacktoberfest (hopefully next year we do!). |
@erickzanardo @renancaraujo @wolfenrain can I get an additional review here? |
Description
Following the conversation on #287, this PR:
min_coverage
defaults to100
, which is a requirement but the behavior is missingmin_coverage
to be between 0 and 100.Type of Change