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

Const-correct ArgumentParser #108

Merged
merged 2 commits into from
Aug 7, 2021

Conversation

Bedzior
Copy link
Contributor

@Bedzior Bedzior commented Aug 2, 2021

Hi!

I wished to provide a small contribution to your fine project.

In another project I am currently working on, I stumbled upon a minor inconvenience in argparse's interface. Normally after parsing the parameters one would expect the parser to be immutable (quite understandably so in simple cases). However, I realized a crucial function checking for a value being actually passed by the user is not, though could be, const!

I am concerned the test I wrote might actually be meaningless. What one would expect there is for the code to actually compile, nothing more, to validate this change. I could also offer rewriting some tests to try to better comply with ArgumentParser's immutability at some point in their bodies, if you deem that viable or useful in any way.

Cheers!

@skrobinson
Copy link
Contributor

@Bedzior

I don't have any say whether this PR is accepted, but I offer a little feedback...

It looks like ::present may have been missed in commit 3734fc6 for #74 and should be const qualified.

Think of your added test as a failing test case that your change to ::present causes to pass. TDD at its best. The test failure is during compilation rather than execution, but it is still a test case.

Are your tests really related to value semantics? I think it would be more fitting to put your tests in a new file (e.g. test_const_correct.cpp).

@p-ranav
Copy link
Owner

p-ranav commented Aug 3, 2021

Agreed with @skrobinson, probably best as a separate test file test_const_correct.cpp. Outside of that, would be happy to merge.

@Bedzior
Copy link
Contributor Author

Bedzior commented Aug 3, 2021

Hi @skrobinson, thanks for throwing an eye!

Think of your added test as a failing test case that your change to ::present causes to pass. TDD at its best. The test failure is during compilation rather than execution, but it is still a test case.

This was my actual thinking when writing this code. However, all checks done under THEN seem excessive, so I am still undecided.

Are your tests really related to value semantics? I think it would be more fitting to put your tests in a new file (e.g. test_const_correct.cpp).

This is my ignorance in relation to the scope of "value semantics". But from what I understand now,const-qualification has its own rightful spot.

@p-ranav it is done

@Bedzior
Copy link
Contributor Author

Bedzior commented Aug 7, 2021

@p-ranav
Is there anything else I could do here?

@p-ranav p-ranav merged commit ccf3920 into p-ranav:master Aug 7, 2021
@Bedzior Bedzior deleted the const-correct-argument-parser branch August 7, 2021 12:45
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.

3 participants