-
Notifications
You must be signed in to change notification settings - Fork 194
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
check_positive
has some contradictions
#686
Conversation
check_positive
has some contradictions
@yarda Hi! Could you please review this PR as soon as possible? Hope to get your feedback |
@superm1 Hi! Could you please review this PR as soon as possible? Hope to get your feedback |
I actually don't have review rights here. I've submitted some code, but one of the maintainers needs to do code review. |
OK! Thanks your reply. |
Thanks, @dufuhang. Looks good to me, but please update the commit message: commit descriptions (first lines) are usually written in imperative present tense, should be reasonably short, and should make it clear where the change is being made. More technical details can be added in the next lines if necessary. You can get inspired by existing commit messages in the repository. |
@zacikpa Thanks your reply! |
1. In the raise statement of the check_positive() function, the error message conflicts with the if condition. 2. If the value of val is 0, it passes the if condition but raises an error message stating [0 has to be >= 0]. 3. This PR removes the = from the raise statement, so when val is 0, it will correctly prompt that the value of val [has to be > 0]. Signed-off-by: dufuhang <[email protected]>
@zacikpa May I ask why this PR has not been merged yet? |
Sorry, that's my fault. It will be merged shortly. I count with it for the December release. Thanks for the contribution. |
When the value passed to
--timeout
is 0, thecheck_positive
function will check the passed value and still prompt that the passed value needs to be greater than or equal to 0. As shown below:You can also change
if <= 0
toif < 0
, but I am not sure whether the--timeout
value can be 0, so I changed the error message instead ofif <= 0
.