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

fix suppressing I/O error in parse_config_file #29

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

JojOatXGME
Copy link
Contributor

@vprus
Copy link
Collaborator

vprus commented Jul 24, 2017

Thanks for the patch, Johannes. I wonder whether it will be even better to do the check inside the

template<class charT>
    basic_parsed_options<charT>
    parse_config_file(std::basic_istream<charT>& is, 
                      const options_description& desc,
                      bool allow_unregistered)

function - that way, the error reporting will be done in all cases, not just when the config file is passed via name.

@JojOatXGME
Copy link
Contributor Author

Since there is currently no way to let parse_config_file(std::basic_istream<charT>& is, ...) throw reading_file, I was not sure whether it is desired behavior to keep the caller responsible for it. Note that the caller might be considered as the owner of the stream. Moreover, the filename is unknown within this function. (It might not even be a file.)

However, since exceptions are already used for various other kinds of errors, it might be more consistent to use them here as well. I do not know what the right solution would be.

@JojOatXGME
Copy link
Contributor Author

JojOatXGME commented Mar 3, 2019

Since there was no progress about this pull request for a long time, I decided to start another attempt. I think it is fine that the caller of

template<class charT>
basic_parsed_options<charT>
parse_config_file(std::basic_istream<charT>& is, 
                  const options_description& desc,
                  bool allow_unregistered)

must check itself if there was an error on the stream. I extended the documentation of this function to make it clear. Throwing an exception could also make the function incompatible with earlier versions which do not throw such exception.

However, an caller of

template<class charT>
basic_parsed_options<charT>
parse_config_file(const char* filename,
                  const options_description&,
                  bool allow_unregistered = false);

cannot check the stream itself. I think it is important to fix this bug and to handle errors while reading the stream.

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@1083bac). Click here to learn what that means.
The diff coverage is 39.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop      #29   +/-   ##
==========================================
  Coverage           ?   49.92%           
==========================================
  Files              ?       23           
  Lines              ?     1388           
  Branches           ?      709           
==========================================
  Hits               ?      693           
  Misses             ?      110           
  Partials           ?      585
Impacted Files Coverage Δ
include/boost/program_options/detail/cmdline.hpp 0% <ø> (ø)
include/boost/program_options/parsers.hpp 40% <ø> (ø)
...lude/boost/program_options/options_description.hpp 0% <ø> (ø)
include/boost/program_options/detail/parsers.hpp 72.97% <100%> (ø)
src/cmdline.cpp 45.51% <100%> (ø)
src/options_description.cpp 48.64% <31.25%> (ø)
include/boost/program_options/errors.hpp 72.82% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1083bac...348ec67. Read the comment docs.

@JojOatXGME
Copy link
Contributor Author

JojOatXGME commented Jul 19, 2020

@vprus I noticed that you have already cherry-picked / rebased my changes for the master branch. This explains why I could not reproduce the issue anymore. This actually confused me quite some time ago. 😄

Is there a reason why

  • the changes don't end up in the develop branch, even after 2 or 3 years, and
  • the pull request was never closed?

Also note that I added a test and made small changes on the documentation later. This changes did not end up in the master branch yet. Not sure if you want to integrate them. Otherwiese, we should maybe close the pull request at some point?

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