Improve nargs #125

hokacci · 2021-09-10T23:05:27Z

Hi.
Thank you for this nice library.

I see some issues regarding "remaining" behavior.

I also wanted optional argument preceded by variable length positional arguments (including zero length) like Python argparse nargs "*", "+", "?"

So, I implemented it just for experiment.
For now, this worked for me.

I know this change drops "remaining" feature, which would impact existing projects.
Also, this seems a complicated problem and I think more study may be needed.

What do you think about this change?

Thank you in advance.

hokacci · 2021-09-11T19:22:04Z

I looked into this more precisely and revived remaining() partially.

The only difference is below.

https://github.com/p-ranav/argparse/pull/125/files#diff-dc3197c89fb7cbd8d7e504f8ac7ffc2cfbae3cc56a1c17e672ec0a96256b61f8L62

skrobinson · 2021-09-13T16:54:05Z

The std::numeric_limits class needs the limits header. Otherwise, tests all pass. Good job.

But, there's many changes here and I may have more questions.

hokacci · 2021-09-14T23:13:35Z

I included "limits" and added some test cases.

Thank you.

p-ranav · 2021-09-15T15:19:11Z

include/argparse/argparse.hpp

+enum class NArgsPattern {
+  ZERO_OR_ONE,
+  ANY,
+  AT_LEAST_ONE
+};


Let's follow the C++ core guidelines and avoid use ALL_CAPS for enumerators.

skrobinson · 2021-09-15T16:17:53Z

Please rebase on current master. While you're rebasing, I'd like to see a reduced number of commits and expanded messages telling why the commit was added. Trying to do per commit reviews was counter-productive due to later commits undoing earlier changes.

Should SizeRange be renamed (e.g. NArgsRange) to indicate its specific purpose? I'm undecided on this and would like to hear from others.

Can the two std::distance checks be shortened? I ask because this looks like duplication to be refactored, but I don't see a simple answer. Maybe it is best as the checks currently work.

I see an opportunity to refactor your updated Argument::validate for DRY and a shorter function. The following example compiles, but fails tests all over the test suite. (And I'm not a fan of my six-condition if, either.)

  void validate() const {
    std::stringstream stream;
    if (mIsOptional) {
      if ((mIsUsed && !mNumArgsRange.contains(mValues.size()) &&
           !mIsRepeatable && !mDefaultValue.has_value()) ||
          (!mNumArgsRange.contains(mValues.size()) && !mDefaultValue.has_value())) {
        if (!mUsedName.empty())
          stream << mUsedName << ": expected ";
        if (mNumArgsRange.is_exact()) {
          stream << mNumArgsRange.get_min();
        } else if (mNumArgsRange.is_right_bounded()) {
          stream << mNumArgsRange.get_min() << " to " << mNumArgsRange.get_max();
        } else {
          stream << mNumArgsRange.get_min() << " or more";
        }
        stream << " argument(s). " << mValues.size() << " provided.";
      } else {
        // TODO: check if an implicit value was programmed for this argument
        if (!mIsUsed && !mDefaultValue.has_value() && mIsRequired) {
          stream << mNames[0] << ": required.";
        }
        if (mIsUsed && mIsRequired && mValues.size() == 0) {
          stream << mUsedName << ": no value provided.";
        }
      }
    }
    if (stream.str().size() > 0)
      throw std::runtime_error(stream.str());
  }

The project's preferred format for constructors appears to be (see Argument ctor):

  SizeRange(std::size_t aMin, std::size_t aMax)
      : mMin(aMin), mMax(aMax) {
    if (aMin > aMax)
      throw std::logic_error("Range of number of arguments is invalid");
  }

The added details::is_container_v<T> check in Argument::get strikes me as another DRY opportunity.

I've learned by reading your code. I'll see something that strikes me as odd. But research shows me why you made that decision and I generally agree with your choice.

To handle variable length nargs, I replaced mNumArgs with mNumArgsRange. I defined SizeRange class for mNumArgsRange, which has simply min and max std::size_t member. To concentrate on this big change, I tentatively deleted remaining feature, which was originally implemented in the way that mNumArgs = -1 internally and maybe_args() -> Optional wrap method. Library users may make use of 4 types of interface to set mNumArgsRange. 1. nargs(std::size_t) 2. nargs(std::size_t, std::size_t) 3. nargs(SizeRange) 4. nargs(NArgsPattern) 1. is expected to behave same as original. This mthod sets min=max SizeRange to mNumArgsRange, which is actually, not a range, but an "exact" number. 2. sets min and max. 3. uses SizeRange class. This interface may be unnecessary. It is also an option to delete this method and make SizeRange class internal. 4. is provided to set common patterns. In Python, they are "?", "*" and "+". NArgsPattern is an enum class for type safety. std::string interface is also an option to mimic Python argparse. char interface would be ambiguous with 1. Changes on consume method is important. The parser tries to consume args until the count reaches mNumArgsRanges::max or it meets another optional like string. If consumed args count is under mNumArgsRanges::min, the parser fails. Now, when the required number of arguments are not provided, the parser will fail. So, we have to take care of get() method as well. get() failed when argument count is 0 and default value not provided. But now there are 0..1 or 0..* nargs are OK. So this behaviour has to be fixed. When T is container_v, it returns empty container. I implemented validate method so that it shows kind message.

I reimplemented remaining() method for backward compatibility. It consumes "all" remaining args. So, I added mAcceptsOptionalLikeValue flag and handle it by using this flag. Currently, remaining() behavior is slightly different from the original when no args are provided and get<Container<T>>() method is called. Originally it raises an exception. But current implementation returns an empty container instead of exception. It is possible to implement complete backward compatibility by referencing mAcceptsOptionalLikeValue flag and raises an exception in get() method, but I did not do this. I think that is too much.

hokacci · 2021-09-15T23:07:52Z

Thank you for your feedback.

I rebased this on master.

I added some explanation below.
I hope this helps you.

hokacci@c6c3be0
hokacci@3d559d3

And I dealt with ALL_CAPS issue and member initializer list preference.

hokacci@bec93ac
hokacci@14abaa4

Later, I will look at other issues you pointed out.

Thank you.

skrobinson · 2021-09-20T17:20:32Z

Thank you for rebasing and consolidating. And your extended commit comments helped me confirm what you intended to do matched the actual changes.

Why do you think it's "too much" to raise an exception in .get()? I'm not disagreeing, I just don't understand why you prefer the new behavior.

In test/test_actions.hpp:

[](std::string &sum, std::string const &value) { sum += value; }

Is there a reason for const after, rather than before, the type?

No rush, but I'd like to see Argument::validate simplified before this branch is merged.

Thank you for the many test cases, they helped me better understand your changes.

EchoPouet · 2022-06-20T13:16:44Z

Hello.
When will this feature be implemented ?

hokacci · 2022-06-20T23:44:40Z

@EchoPouet
Hi, thank you for reminding me of this PR.
I was so busy and did not have enough time for this PR.

@skrobinson

Why do you think it's "too much" to raise an exception in .get()? I'm not disagreeing, I just don't understand why you prefer the new behavior.

It seemed natural for me. But now I think compatibility counts. So I fixed it.

Is there a reason for const after, rather than before, the type?

No. Seemingly this project uses pre-const style. Now I also took this style.

No rush, but I'd like to see Argument::validate simplified before this branch is merged.

I wrote some helper functions and make validate() clearer.

Also, I merged current master branch and fixed many conflicts.

Thank you.

EchoPouet · 2022-06-21T15:25:28Z

Thank you @hokacci. I'm looking forward to the merge and the release.

p-ranav · 2022-06-21T17:08:22Z

include/argparse/argparse.hpp

+};
+
+enum class NArgsPattern {
+  ZeroOrOne,


For consistency, let's use lower_case underscore-separated field names for enum classes.

Fixed in b869b5a

p-ranav · 2022-06-21T17:13:56Z

I'd also love for the README to be updated as part of the PR, to describe the new version of .nargs(...).

skrobinson · 2022-06-21T19:27:57Z

There have been many changes since you branched from the main line. Some of the current merge conflict fixes undo recent changes, e.g. commit 37a1f3b in PR #172 and commit 748bc95 in PR #142. I'd like to see this feature added without preventable side-effects.

hokacci · 2022-06-21T23:02:14Z

I'd also love for the README to be updated as part of the PR, to describe the new version of .nargs(...).

@p-ranav
I added minimal explanation in e44023f

hokacci · 2022-06-21T23:05:36Z

@skrobinson

There have been many changes since you branched from the main line. Some of the current merge conflict fixes undo recent changes, e.g. commit 37a1f3b in PR #172 and commit 748bc95 in PR #142. I'd like to see this feature added without preventable side-effects.

For commit 37a1f3b, I retrieved it in 5d6544a
It was my mistake. Sorry.

For commit 748bc95, it seems the change is already retrieved.

Thank you.

hokacci · 2022-06-22T00:33:30Z

I also did some small fixes in the recent three commits.
nargs_pattern::zero_or_one -> nargs_pattern::optional
SizeRange -> NArgsRange
and moved NArgsRange to private in Argument class. Users don't need to use this class.

skrobinson

This looks good to me. Thank you, @hokacci.

skrobinson mentioned this pull request Sep 14, 2021

Make get syntax flexible #127

Merged

p-ranav reviewed Sep 15, 2021

View reviewed changes

hokacci force-pushed the feature/variable-length-nargs branch 2 times, most recently from d5685bb to 13f0939 Compare September 15, 2021 21:59

hokacci added 10 commits September 16, 2021 07:22

Remove negative parameter test for now unsigned

c99272b

Fix tests for remaining

10ddd39

Change test for remaining to test for * nargs

2cfe115

Add another test for remaining

aae2e93

Add test for variable nargs

bb115c0

Add test for reversed order nargs

8845260

Avoid use ALL_CAPS for enumerators

bec93ac

Use member initializer list to SizeRange ctor

14abaa4

hokacci force-pushed the feature/variable-length-nargs branch from 2cc2ccf to 14abaa4 Compare September 15, 2021 23:01

hokacci added 5 commits June 21, 2022 06:44

Restore a "remaining" test case for compat

6dfaa1c

Complete "remainig" backward compatibility

0195a50

Prefer pre-const to post-const

12fcae6

Make throw_* funcs and make validate() clearer

3459eec

Merge branch 'master' into feature/variable-length-nargs

08943f4

p-ranav reviewed Jun 21, 2022

View reviewed changes

hokacci added 5 commits June 22, 2022 07:07

NArgsPattern -> nargs_pattern (to snake case)

b869b5a

Retrieve changes on 37a1f3b

5d6544a

Prefer empty() to size() == 0

df6e7de

Fix a typo

7b5084c

Explain variable-length arguments in README

e44023f

hokacci added 3 commits June 22, 2022 09:24

Use optional instead of zero_or_one

acff046

SizeRange -> NArgsRange

25d24c7

Move NArgsRange to private: because it is detail of implementation

ed84d90

skrobinson approved these changes Jun 22, 2022

View reviewed changes

p-ranav merged commit 24c599d into p-ranav:master Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve nargs #125

Improve nargs #125

hokacci commented Sep 10, 2021

hokacci commented Sep 11, 2021

skrobinson commented Sep 13, 2021

hokacci commented Sep 14, 2021

p-ranav Sep 15, 2021

skrobinson commented Sep 15, 2021

hokacci commented Sep 15, 2021

skrobinson commented Sep 20, 2021

EchoPouet commented Jun 20, 2022

hokacci commented Jun 20, 2022 •

edited

Loading

EchoPouet commented Jun 21, 2022

p-ranav Jun 21, 2022

hokacci Jun 21, 2022

p-ranav commented Jun 21, 2022

skrobinson commented Jun 21, 2022

hokacci commented Jun 21, 2022

hokacci commented Jun 21, 2022

hokacci commented Jun 22, 2022

skrobinson left a comment

Improve nargs #125

Improve nargs #125

Conversation

hokacci commented Sep 10, 2021

hokacci commented Sep 11, 2021

skrobinson commented Sep 13, 2021

hokacci commented Sep 14, 2021

p-ranav Sep 15, 2021

Choose a reason for hiding this comment

skrobinson commented Sep 15, 2021

hokacci commented Sep 15, 2021

skrobinson commented Sep 20, 2021

EchoPouet commented Jun 20, 2022

hokacci commented Jun 20, 2022 • edited Loading

EchoPouet commented Jun 21, 2022

p-ranav Jun 21, 2022

Choose a reason for hiding this comment

hokacci Jun 21, 2022

Choose a reason for hiding this comment

p-ranav commented Jun 21, 2022

skrobinson commented Jun 21, 2022

hokacci commented Jun 21, 2022

hokacci commented Jun 21, 2022

hokacci commented Jun 22, 2022

skrobinson left a comment

Choose a reason for hiding this comment

hokacci commented Jun 20, 2022 •

edited

Loading