-
Notifications
You must be signed in to change notification settings - Fork 350
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
Release update #974
Release update #974
Conversation
@henryiii I think it is time to make a release, been over a year since the the last one, and some bigger features in place that should be in a release version. Is there anything else you want in a release? I know you talked about something with Unicode but not sure what exactly that was. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #974 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 4546 4546
=========================================
Hits 4546 4546 ☔ View full report in Codecov by Sentry. |
@henryiii pinging you again if there is anything else in a release and to check over the changelog. |
The unicode thing I'd like to check before release was here, I think. Basically, now we have (2), (3), and (4): // 1 (not supported today)
app.ensure_utf8(argc, argv);
CLI11_PARSE(app); // Uses stored argv vector on all platforms
// 2 (almost supported in main only - missing argc)
argv = app.ensure_utf8(argc, argv);
CLI11_PARSE(app, argc, argv); // Uses produced argv vector
// 3 (supported in all versions)
CLI11_PARSE(app, argc, argv); // Not unicode
// 4 (supported in main only)
CLI11_PARSE(app); // Not portable, but unicode I'm not fond of (4), since it requires non-standard trickery that sometimes fails, and the reluctance to release is because we'd be making that a supported way to operate. We could get rid of this and add (1), which is a bit more code, but doesn't have any non-standard trickery and is still simpler than (2). We could also leave it as is. We could also just remove (4), and only allow (2) and (3), as more ways to do things just to save a few characters isn't necessarily better. |
Actually, we currently have: argv = app.ensure_utf8(argv);
CLI11_PARSE(app, argc, argv); // Uses produced argv vector Which is not compatible with (1) at all. |
Here are the possibilities, IMO:
|
Longer term we should figure out the best means of not using MACROS as recommended. It does serve a purpose so fine leaving it for now. But in my opinion 1 and 4 should not be supported macros, they hide too much. I don't see a good means of getting rid of them completely in C++11 so there should only be 1 form of the macro, that is fairly simple. |
What we are really talking about whether to keep the |
Yes, it's really about So you like Option 2? |
I think that would be my preference but I don't use much unicode, |
Looking at it, I think the simplest immediate solution would be rename the parse() method to something else e.g. |
I'm looking at the code removable if we drop the no-argument IMO, having |
(Currently distracted by fixing Catch2 3 support, which seems to have broken) |
@henryiii any chance we can finish this up this week. I am working on a release for another project and would be nice to include an actual release of CLI11 in it. |
We can try. I’m likely to not have time tomorrow, but Thursday Friday I should. I’m still very worried about Unicode and want to strip the “simple” form out before release. I’d also like to know if it supports wmain on windows, and what happens if you aren’t expecting Unicode. |
Anything we need to change in this PR? @henryiii |
I'll try to do final review this weekend, release on Monday. |
1310464
to
7b34280
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
63e7b05
to
3bdebd8
Compare
Update the Readme and the changelog in preparation for a release.