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.

Sign up for GitHub

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

Jump to bottom

[dnf5] Fix help in case argument parser detect error #450

Merged
merged 2 commits into from
Apr 13, 2023
Merged

[dnf5] Fix help in case argument parser detect error #450

merged 2 commits into from
Apr 13, 2023

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Apr 12, 2023 •
edited
Loading

  • if the user wants help (--help/-h argument is present) -> print help and only help without errors and the application ends with the return code libdnf::cli::ExitCode::SUCCESS.
  • if user doesn't want help (--help/-h argument is not present) and argument parser detects missing/bad argument -> print an error message with a note about '--help' and application exits with return code libdnf::cli::ExitCode::ARGPARSER_ERROR

Solves: #317

Requires modification of CI tests: rpm-software-management/ci-dnf-stack#1247

@jrohel
Copy link
Contributor Author

jrohel commented Apr 12, 2023 •
edited
Loading

This PR is OK. According to the output, there is a bug in the CI tests. The "resolve" option is really missing in the last command.

Last Command: dnf5 -y --installroot=/tmp/dnf_ci_installroot_ze5yse_k --releasever=29 --setopt=module_platform_id=platform:f29 --setopt=reposdir=/tmp/dnf_ci_installroot_ze5yse_k/etc/yum.repos.d --setopt=config_file_path=/tmp/dnf_ci_installroot_ze5yse_k/etc/dnf/dnf.conf --setopt=cachedir=/tmp/dnf_ci_installroot_ze5yse_k/var/cache/dnf download abcde --alldeps

Last Command stderr:
Option alldeps should be used with resolve

@jrohel jrohel mentioned this pull request Apr 12, 2023
Closed
praiskup
praiskup reviewed Apr 12, 2023
View reviewed changes
dnf5/main.cpp Outdated
return static_cast<int>(libdnf::cli::ExitCode::SUCCESS);
}
}
std::cerr << ex.what() << ". Add \"--help\" for more information about the arguments." << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Can you wrap this literal with the gettext _( ) wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added. This reminded me to look at localization in dnf5 in general.

@m-blaha
Copy link
Member

m-blaha commented Apr 12, 2023 •
edited
Loading

I think that's what the scenario tests - --alldeps without --resolve prints error message.
But the real problem is different exit code. Command exited with 1 but 2 (we use this value for argparsing errors) was expected.

praiskup
praiskup reviewed Apr 12, 2023
View reviewed changes
dnf5/main.cpp
return static_cast<int>(libdnf::cli::ExitCode::ARGPARSER_ERROR);
} catch (libdnf::cli::ArgumentParserError & ex) {
std::cerr << ex.what() << std::endl;
return static_cast<int>(libdnf::cli::ExitCode::ARGPARSER_ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

Removing the catch statements evokes that ArgumentParserError may actually never be raised in the corresponding try statement. Does it mean that we should never ever throw it in command->run() etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@praiskup I thought about it. There is at least one case where we throw an exception in command->configure (a problem with the download command). So I will modify the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m-blaha Good point. Thanks.

@jrohel
Copy link
Contributor Author

jrohel commented Apr 13, 2023

CI tests fails again:

  1. The original code in some cases writes the argument parser error to stdout instead of stderr. I think it was a bug. Now all arguments parser errors are written to stderr. (search command)
  2. A test using an exact match cannot pass because a note about the --help argument is added. (download command)

@jrohel
[dnf5] Fix help in case argument parser detect error
bee547a 
- if the user wants help (--help/-h argument is present) -> print help
  and only help without errors and the application ends with the return
  code libdnf::cli::ExitCode::SUCCESS.
- if user doesn't want help (--help/-h argument is not present) and
  argument parser detects missing/bad argument -> print an error message
  with a note about '--help' and application exits with return code
  libdnf::cli::ExitCode::ARGPARSER_ERROR
@m-blaha m-blaha self-assigned this Apr 13, 2023
@jrohel jrohel mentioned this pull request Apr 13, 2023
Merged
m-blaha
m-blaha approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@m-blaha m-blaha 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, LGTM.

@m-blaha m-blaha merged commit 293e431 into rpm-software-management:main Apr 13, 2023
@jrohel jrohel mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@praiskup praiskup praiskup left review comments

@m-blaha m-blaha m-blaha approved these changes

Assignees

@m-blaha m-blaha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jrohel @m-blaha @praiskup