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

Better ways to deal with data type? #225

Open
lingerer opened this issue Oct 19, 2022 · 2 comments
Open

Better ways to deal with data type? #225

lingerer opened this issue Oct 19, 2022 · 2 comments

Comments

@lingerer
Copy link

lingerer commented Oct 19, 2022

After an agumentParse parse successful,We can get the value:


 template <typename T>
  auto get() const
      -> std::conditional_t<details::IsContainer<T>, T, const T &> {
    if (!m_values.empty()) {
      if constexpr (details::IsContainer<T>) {
        return any_cast_container<T>(m_values);
      } else {
        return *std::any_cast<T>(&m_values.front());
      }
    }
    if (m_default_value.has_value()) {
      return *std::any_cast<T>(&m_default_value);
    }
    if constexpr (details::IsContainer<T>) {
      if (!m_accepts_optional_like_value) {
        return any_cast_container<T>(m_values);
      }
    }

    throw std::logic_error("No value provided for '" + m_names.back() + "'.");
  }

The actual values are stored in m_values(vector) and by m_default_value (when m_default_value is empty)
In my practices , it may result some inconsistent result.
It's a long story,I will try to make it clear through my path.
1.nullptr error
If you just begin to try argparse,you will glad to see this will work
test --arg 12

test.add_argument("--arg");
test.parse_args(argc, argv);
auto s_arg = test.get<string>("--arg");
std::cout<<"arg:"<<s_arg<<std::endl;

but this will result nullptr error

test.add_argument("--arg");
test.parse_args(argc, argv);
auto s_arg = test.get<int>("--arg");
std::cout<<"arg:"<<s_arg<<std::endl;

get will return a nullptr exception cause by return *std::any_cast<T>(&m_values.front());,the exception will not be catch .
So my first suggestion is catch the any_cast exception before return,if any_cast fail just throw the std::logic_error.

2.value type problem?
Then I dig more

test.add_argument("--arg").default_value(1.1);
test.parse_args(argc, argv);
auto f_arg = test.get<float>("--arg");
auto d_arg = test.get<double>("--arg");

auto f_arg = test.get<float>("--arg"); will result nullptr and auto d_arg = test.get<double>("--arg"); won't.
1.1 is treat as double so that's why get<float> will result nullptr exception.
test.add_argument("--arg").default_value(float(1.1)); will make get<float> work but get<double> failed,so that's all about std::any type.
But... what if this code
test.add_argument("--arg").default_value(1.1).scan<'f',float>();
default value will be treat as double and scanner will convert to float.It will result great confusion in practices.
So my second suggestion is force a type compare between default value and scanner or provide a default scanner when provide default_value.
This issue is m_default_value m_values may contain different data type.

3.More thoughts
When I found almost all the pains is due to how to cast string argument to std::any and then cast back to datatype and the possible data type different between m_default_value and m_values.Why not provide a way just store the string argument? Is it possible or it just cost too much for common use?

@skrobinson
Copy link
Contributor

skrobinson commented Oct 27, 2022

1 You surprised me with this one because failures on that line used to throw std::bad_any_cast. I believe that is still the proper exception. There was a fix for this mixed in with other changes that were not accepted in PR #199. We should try to resurrect those two lines.

2 My first reaction is: "don't do that". Your examples act that way because the chosen values are misleading the compiler. 1.1 is a double, 1.1f is a float.

IMHO, developers should be aware of their types. Yes, when I first began using argparse, I caused similar problems for myself. But, these errors told me to be more careful about the value type going in and coming out.

3 Storing the string and delaying the conversion means the developer's error may not be shown until very late.

@lingerer
Copy link
Author

2 The main suggestion is the default value should apply scanner as well,not only value.
I can't offer a code patch cause I'm a old dog only can read but not able to produce a template code.

skrobinson added a commit to skrobinson/argparse that referenced this issue Jan 18, 2023
This attempts to fix Issue p-ranav#225-1 by reverting the change that turned a
std::bad_any_cast exception into a nullptr.

Reverts commit 3570681.

Signed-off-by: Sean Robinson <[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

No branches or pull requests

2 participants