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

Allow non standard option names like -option #1078

Merged
merged 15 commits into from
Oct 23, 2024

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Oct 19, 2024

This has been bounced around for a couple years now

#474 and a few others have expressed desire to work with non-standard option names. We have been somewhat resistant to that but I think it can be done now. This PR adds a modifier allow_non_standard_option_names() It is purposely long, it is purposely off by default. But what it does is allow option names with a single - to act like a short option name. With this modifier enabled no single letter short option names are allowed to start with the same letter as a non-standard names. For example -s and -single would not be allowed.

Copy link

codecov bot commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e4ee3af) to head (0d5e42e).
Report is 48 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #1078     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           17        17             
  Lines         4546      4971    +425     
  Branches         0      1022   +1022     
===========================================
+ Hits          4546      4971    +425     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phlptp phlptp requested a review from henryiii October 21, 2024 02:37
@phlptp
Copy link
Collaborator Author

phlptp commented Oct 21, 2024

@henryiii Been thinking about this one for a while. played with a bunch of different options for this, still not sure how I feel about it, but I do see the point that if CLI11 is to be a replacement in some legacy applications, we do need some mechanic to support single dash long options. This way seems like the least intrusive and least complex on the option side. Not going to merge this until you approve though.

@@ -22,7 +22,7 @@ jobs:
- job: CppLint
pool:
vmImage: "ubuntu-latest"
container: sharaku/cpplint:latest
container: helics/buildenv:cpplint
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the container we had been using hasn't been updated in 5 years. Figured we needed something a little newer.

README.md Outdated Show resolved Hide resolved
@@ -9,6 +9,7 @@
#include <CLI/CLI.hpp>
#include <iostream>
#include <sstream>
#include <string>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these adding string with no changes? IWYU?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the update to cpplint includes a lot more IWYU checks so this came up.

#include <vector>

using namespace boost::container;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ (change not related to this PR's topic, but like the change)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IWYU was confusing slist from boost with the one from gcc, obviously couldn't include that one, so that just triggered getting rid of the boost container namespace and using it more directly with the full name of the types

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@phlptp phlptp merged commit 5a03ee5 into CLIUtils:main Oct 23, 2024
56 checks passed
@phlptp phlptp deleted the non_standard_names branch October 23, 2024 12:14
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