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

missing function definition #793

Merged
merged 9 commits into from
Oct 28, 2022
Merged

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Oct 27, 2022

the get_option_group definition was missing from the splitting of the definitions.

I tried to compile the single header into our code and discovered a missing function call.

@phlptp phlptp added the bug label Oct 27, 2022
@phlptp
Copy link
Collaborator Author

phlptp commented Oct 28, 2022

Need to merge #794 first, that should fix the tests

@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Base: 99.42% // Head: 99.42% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e8cce4e) compared to base (4dbe4b4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #793   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          16       16           
  Lines        3972     3977    +5     
=======================================
+ Hits         3949     3954    +5     
  Misses         23       23           
Impacted Files Coverage Δ
include/CLI/impl/App_inl.hpp 99.20% <100.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@henryiii
Copy link
Collaborator

Ouch, is this a regression? Do we know why it was passing tests?

@phlptp
Copy link
Collaborator Author

phlptp commented Oct 28, 2022

I think the recent pre-commit update changed something and introduced a cyclic change in some files that pre-commit couldn't resolve on its own. And I didn't want to make a bunch of unrelated doc changes in this PR. As to the original problem yes I think it was a regression but I am still trying to figure out why this wasn't triggering some issues with the single header tests or the other tests for that matter. What I know is tried updating the single header CLI11 used in other applications and it generated a linking error which led to a missing definition.

@henryiii
Copy link
Collaborator

It does complain that it's completely untested.

@henryiii
Copy link
Collaborator

And I'm not seeing a test for it.

@phlptp
Copy link
Collaborator Author

phlptp commented Oct 28, 2022

I will add some tests

@henryiii
Copy link
Collaborator

henryiii commented Oct 28, 2022

Looks like we can do a .1 release after this, no new features have gone in yet AFAICT.

@phlptp
Copy link
Collaborator Author

phlptp commented Oct 28, 2022

as long as we do it before merging #789 and #786, that should be fine.

@henryiii henryiii merged commit 25274e2 into CLIUtils:main Oct 28, 2022
@henryiii henryiii deleted the missing_app_function branch October 28, 2022 13:57
@github-actions github-actions bot added needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Oct 28, 2022
@henryiii henryiii removed the needs README Needs to be mentioned in the README label Oct 28, 2022
@henryiii henryiii removed the needs changelog Hasn't been added to the changelog yet label Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants