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

[MISC] Add new output_file_validator ctors #71

Merged
merged 1 commit into from
Mar 21, 2022
Merged

[MISC] Add new output_file_validator ctors #71

merged 1 commit into from
Mar 21, 2022

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Mar 16, 2022 •
edited
Loading

Open:

  • We probably want some check (static_assert) for construction the vector from the parameter pack:
    • We need requires as it's important for overload resultion.
  • More/different documentation?

@vercel
Copy link

vercel bot commented Mar 16, 2022 •
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/sharg-parser/ZGyekBpXQd5Fwwz61GZk8xrRZwkE
✅ Preview: https://sharg-parser-git-fork-eseiler-misc-outputfile-seqan.vercel.app

@eseiler eseiler mentioned this pull request Mar 16, 2022
Closed
2 tasks
@eseiler eseiler linked an issue Mar 16, 2022 that may be closed by this pull request
out_file extensions doesn't work #63
Closed
2 tasks
@codecov
Copy link

codecov bot commented Mar 16, 2022 •
edited
Loading

Codecov Report

Merging #71 (3865241) into master (6bc0706) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   94.63%   94.67%   +0.04%     
==========================================
  Files          14       14              
  Lines        1194     1203       +9     
==========================================
+ Hits         1130     1139       +9     
  Misses         64       64
Impacted Files Coverage Δ
include/sharg/validators.hpp 100.00% <100.00%> (ø)

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 6bc0706...3865241. Read the comment docs.

h-2
h-2 reviewed Mar 16, 2022
View reviewed changes
h-2
h-2 reviewed Mar 16, 2022
View reviewed changes
@eseiler eseiler force-pushed the misc/output_file branch 2 times, most recently from 7776b34 to c45770a Compare March 16, 2022 11:54
@eseiler eseiler requested a review from h-2 March 16, 2022 11:56
h-2
h-2 requested changes Mar 16, 2022
View reviewed changes
include/sharg/validators.hpp Outdated
* \param[in] mode A sharg::output_file_open_options indicating whether the validator throws if a file already
* exists.
* \param[in] extensions Parameter pack representing valid extensions. std::string must be constructible from each
* parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* parameter.
* argument. The pack may be empty ( → all extensions are valid).

@eseiler eseiler requested a review from h-2 March 16, 2022 14:10
h-2
h-2 approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@h-2 h-2 left a comment

Choose a reason for hiding this comment

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

LGTM

@eseiler eseiler requested a review from smehringer March 17, 2022 16:25
smehringer
smehringer requested changes Mar 21, 2022
View reviewed changes
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

LGTM. Just a missing test case I think

include/sharg/validators.hpp
Comment on lines +616 to +620
/*!\brief Constructs from a list of valid extensions.
* \param[in] extensions The valid extensions to validate for.
*/
explicit output_file_validator(std::vector<std::string> const & extensions)
: output_file_validator{output_file_open_options::create_new, extensions}
{}
Copy link
Member

@smehringer smehringer Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If I use this constructor and an empty list, are all extensions valid then? (should be, just wondering about the implementation because I think a test case is missing?)

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 the same, because we do file_validator_base::extensions = std::move(extensions); and file_validator_base::extensions is empty by default.

I added explicit tests :)

@eseiler eseiler requested a review from smehringer March 21, 2022 14:14
@smehringer smehringer enabled auto-merge March 21, 2022 14:21
@smehringer smehringer merged commit ff5db8e into seqan:master Mar 21, 2022
@eseiler eseiler deleted the misc/output_file branch March 21, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-2 h-2 h-2 approved these changes

@smehringer smehringer smehringer 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.

out_file extensions doesn't work
3 participants
@eseiler @h-2 @smehringer