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.

Sign up for GitHub

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

Jump to bottom

[FIX] default container arguments #2394

Merged
merged 1 commit into from
Feb 23, 2021
Merged

[FIX] default container arguments #2394

merged 1 commit into from
Feb 23, 2021

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Feb 19, 2021

Resolves #2393

@eseiler eseiler requested a review from Irallia February 19, 2021 07:49
@vercel
Copy link

vercel bot commented Feb 19, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/izilc2ddo
✅ Preview: https://seqan3-git-fork-eseiler-fix-argparsedefault.seqan.vercel.app

@codecov
Copy link

codecov bot commented Feb 19, 2021

Codecov Report

Merging #2394 (45db946) into master (4355a95) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2394   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         266      266           
  Lines       10843    10846    +3     
=======================================
+ Hits        10650    10653    +3     
  Misses        193      193
Impacted Files Coverage Δ
...ude/seqan3/argument_parser/detail/format_parse.hpp 96.84% <100.00%> (+0.04%) ⬆️

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 4355a95...45db946. Read the comment docs.

@eseiler eseiler mentioned this pull request Feb 19, 2021
Merged
Irallia
Irallia approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

LGFM, thank you!

@Irallia Irallia requested a review from marehr February 22, 2021 12:11
marehr
marehr approved these changes Feb 23, 2021
View reviewed changes
include/seqan3/argument_parser/detail/format_parse.hpp
{
assert(positional_option_count == positional_option_calls.size()); // checked on set up.

value.clear();
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand, why we have two different functions to set a value.

I would have expected to only have one point to change this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's once for positional options and once for options

@marehr marehr merged commit ecda488 into seqan:master Feb 23, 2021
@eseiler eseiler deleted the fix/argparsedefault branch May 14, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marehr marehr marehr approved these changes

@Irallia Irallia Irallia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Argument Parser cannot have default container arguments
3 participants
@eseiler @marehr @Irallia