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

Clear previous parse results before starting a new parse #5

Merged
merged 1 commit into from
May 13, 2018
Merged

Clear previous parse results before starting a new parse #5

merged 1 commit into from
May 13, 2018

Conversation

pavelkryukov
Copy link
Contributor

These changes are another outcome of integrating Popl into our unit tests. The idea of our tests is to set up options once during static initialization, and check whether different arrays of arguments are parsed to a meaningful output or not.

Thus we need to perform a cleanup values before parse starts, but keep names and types of each option. As I see, the "non-options" results are cleared already, so I put my code nearby to it.

Cleaning ValueTemplate is straightforward, however I don't understand if count_ cannot be equal to options_.size(). If yes, it might be removed.

Switch is more tricky, as it updates remote value in ctor. I changed its behavior, so on clears it assigns to false, and does otherwise on parse. In my implementation, options_ vector of Switch is not able to contain more than 1 element, however I consider that as a natural thing, as there are only two states possible for it (switch exists in argv or it does not)

@badaix
Copy link
Owner

badaix commented May 13, 2018

I didn't review it deeply, but by just compiling the example, it seems that the switch is implicitly set when using this PR:

~/Develop/popl/build $ ./popl_example
Allowed options:
  -h, --help                   produce help message
  -v, --verbose                be verbose
  -d, --double arg (=3.14159)  test for double values
  -f, --float arg (=2.71828)   test for float values
  -i, --int arg (=23)          test for int value w/o option
  -s, --string arg             test for string values
  -m, --implicit [=arg(=42)]   implicit test

verbose_option  - is_set: 1, count: 1, reference: 0
hidden_option   - is_set: 1, count: 1
double_option   - is_set: 0, count: 0, value: 3.14159
...

but output without any command line flag should be:

~/Develop/popl/build $ ./popl_example 
verbose_option  - is_set: 0, count: 0, reference: 0
hidden_option   - is_set: 0, count: 0
double_option   - is_set: 0, count: 0, value: 3.14159
...

also the count will not go higher than one. When using -vvv the verbose count should be 3, but is 1.

@badaix
Copy link
Owner

badaix commented May 13, 2018

I think you can actually leave the Switch untouched.
OptionParser::parse will clear all options, i.e. will clear the switch values_ and set count = 0, which makes the switch "not set".
Also there seems to be a bug in the current ValueTemplate<T>::set_value function. It calls values_.clear(); and add_value(value);. It should rather call the new ValueTemplate<T>::clear() function, which will also correctly reset the count_ to 0, followed by add_value(value);.
Only problematic case are the assign_to pointers, which should be set to the default value (if there is any)

@badaix badaix merged commit 179c8dd into badaix:develop May 13, 2018
@badaix
Copy link
Owner

badaix commented May 13, 2018

I merged and removed the clear() from Switch. I need to think about the default and assign_to stuff.
I think a Switch should have a fixed default (false).
This might change the whole class structure (maybe the ValueTemplate class will not be needed anymore)

@pavelkryukov
Copy link
Contributor Author

pavelkryukov commented May 13, 2018

OK, it's up to you.

I've implemented one more feature that my project needs, a "mandatory" or "required" option: it must be set from command line, otherwise an exception is thrown. It is useful for making a "filename" option, as you cannot set any default value it. Actually it serves as intermediate level between ValueTemplate and Value, and it can be merged with ValueTemplate completely. I'll push it to my branch, but it might be not 100% completed yet.

@pavelkryukov
Copy link
Contributor Author

pavelkryukov commented May 13, 2018

I merged and removed the clear() from Switch. I need to think about the default and assign_to stuff.
I think a Switch should have a fixed default (false).

That does not work in my environment. If Switch is assignes a remote value at the 1st parse, it is still assigned to true after 2nd parse, regardless of its input.

@badaix
Copy link
Owner

badaix commented May 14, 2018

did you try with the current version? 722a55d

@pavelkryukov
Copy link
Contributor Author

Thanks, works well now!

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.

None yet

2 participants