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

Third stage callback #313

Merged
merged 3 commits into from
Sep 4, 2019
Merged

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Aug 22, 2019

If merged this PR will add a third callback to apps.

Based on the discussion from gitter

It adds two new function final_callback and parse_complete_callback The `callback function will populate one of these depending on the status of immediate_callaback. immediate_callback will also toggle between them if one is empty. The default is routing to final_callback which has the same behavior as prior to the PR.

Some thoughts about deprecating the immediate_callback but losing it would remove capabilities at present, so I don't think it should be deprecated yet at least until the callback function itself is deprecated.

@phlptp
Copy link
Collaborator Author

phlptp commented Aug 22, 2019

I want to see where additional test coverage is needed for this PR, so I think it ready for review, but I will probably need to add some more tests yet.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #313 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #313   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        3017   3027   +10     
=====================================
+ Hits         3017   3027   +10
Impacted Files Coverage Δ
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06ab2d0...4af6d49. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #313 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #313   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        3028   3038   +10     
=====================================
+ Hits         3028   3038   +10
Impacted Files Coverage Δ
include/CLI/Option.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/ConfigFwd.hpp 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc6e1c7...d36dfe9. Read the comment docs.

add test from readme about callbacks

update callback documentation,

add a subbcommand immediate_callback test

add third callback and readme update
@phlptp phlptp force-pushed the third_stage_callback branch from 4af6d49 to 627837a Compare September 3, 2019 13:02
@phlptp
Copy link
Collaborator Author

phlptp commented Sep 3, 2019

I think everything is ready to go here. I left immediate_callback operation in place to avoid any breaking changes. So callback should behave as before even though the operation is technically redundant. immediate_callback still functions from a user perspective as before even though the actual operations it does are very different.

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

I think this would really benefit from having the tutorial book updated. I'll try to merge that into CLI11 in the next week or two.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Co-Authored-By: Henry Schreiner <HenrySchreinerIII@gmail.com>
@phlptp
Copy link
Collaborator Author

phlptp commented Sep 3, 2019

Keeping the book up to date will probably be a lot easier once it is merged. I know I would be more inclined to update that if it was in a single repository.

@henryiii henryiii merged commit cf4933d into CLIUtils:master Sep 4, 2019
@henryiii henryiii deleted the third_stage_callback branch September 4, 2019 02:46
@henryiii henryiii added this to the v1.9 milestone Dec 31, 2019
@phlptp phlptp mentioned this pull request Jan 1, 2020
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