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

check_positive has some contradictions #686

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

Conversation

dufuhang
Copy link

@dufuhang dufuhang commented Sep 4, 2024

When the value passed to --timeout is 0, the check_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:

ubuntu@VM-24-14-ubuntu:~/github/tuned$ tuned-adm --timeout 0
usage: tuned-adm [-h] [--version] [--debug] [--async] [--timeout TIMEOUT] [--loglevel LOGLEVEL] {list,active,off,profile,profile_info,recommend,verify,auto_profile,profile_mode} ...
tuned-adm: error: argument --timeout/-t: 0 has to be >= 0

You can also change if <= 0 to if < 0, but I am not sure whether the --timeout value can be 0, so I changed the error message instead of if <= 0.

@dufuhang dufuhang changed the title 'check_positive' has some contradictions check_positive has some contradictions Sep 4, 2024
@dufuhang
Copy link
Author

dufuhang commented Sep 5, 2024

@yarda Hi! Could you please review this PR as soon as possible? Hope to get your feedback

@dufuhang
Copy link
Author

@superm1 Hi! Could you please review this PR as soon as possible? Hope to get your feedback

@superm1
Copy link
Contributor

superm1 commented Sep 13, 2024

@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.

@dufuhang
Copy link
Author

@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.

@zacikpa
Copy link
Contributor

zacikpa commented Sep 13, 2024

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.

@dufuhang
Copy link
Author

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!
I have updated the commit message. Please review it again. Thank you.

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]>
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.

3 participants