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

[FEATURE] Add sharg-parser submodule and replace the seqan3::argument_parser. #2954

Closed
wants to merge 15 commits into from

Conversation

smehringer
Copy link
Member

@smehringer smehringer commented Mar 27, 2022

Blocked by #2952 #2970

This PR does a lot.

  • it adds the sharg parser as submodule
  • it fixes a test in format_parse_validator_test where enum foo needs an enumeration_names overload in order to work with validators and the argument parser. This was not enforced by the validator itself, but is enforced in sharg. Since you don't use validators without the argument_parser this can be considered a fix not an API break.
    • A note in the changelog should still be added I guess
  • It replaces the help page tests in format_parse_validator_test by comparing the output with filter views instead of accessing the private member console_layout to set the terminal width, s.t. the output is comparable. The sharg::detail::help+format would otherwise need to befriend the seqan3 test accessor but this would add a dependency on seqan3.
  • It deletes all detail files since they are not for the user and are not needed in seqan3
  • It replaces all public seqan3 API by sharg API via using namespace sharg; declaration. The following exceptions had to be made:
    • The argument_parser class will have differences to the sharg parser and thus must inherit from it.
    • The input and output validators had I/O specific functionality that was removed in sharg and added again here to avoid breaking code for now. We can discuss if we want to deprecate them completely.
  • The enumeration_names CPO can be satisfied by overloading seqan3::custom::argument_parsing. This must be done with the explicit, complete namespace instead of opening the namespace seqan3::custom{ ... }.
    • Adjust snippet or tutorial

Once everything compiles I will make separate commits for reviews

@vercel
Copy link

vercel bot commented Mar 27, 2022

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/7akSVV74g9NzS31FN6ytVf37692e
✅ Preview: https://seqan3-git-fork-smehringer-sharg-seqan.vercel.app

[Deployment for 9daf967 failed]

@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #2954 (bbeac7e) into master (d9294eb) will increase coverage by 0.38%.
The diff coverage is 100.00%.

❗ Current head bbeac7e differs from pull request most recent head e4a3969. Consider uploading reports for the commit e4a3969 to get more accurate results

@@            Coverage Diff             @@
##           master    #2954      +/-   ##
==========================================
+ Coverage   98.21%   98.59%   +0.38%     
==========================================
  Files         274      258      -16     
  Lines       12216    10336    -1880     
==========================================
- Hits        11998    10191    -1807     
+ Misses        218      145      -73     
Impacted Files Coverage Δ
include/seqan3/argument_parser/argument_parser.hpp 100.00% <100.00%> (+2.51%) ⬆️
include/seqan3/argument_parser/validators.hpp 100.00% <100.00%> (ø)
include/seqan3/core/debug_stream/variant.hpp 80.00% <0.00%> (-5.72%) ⬇️
...clude/seqan3/search/detail/search_configurator.hpp 93.33% <0.00%> (-3.64%) ⬇️
...an3/alignment/scoring/aminoacid_scoring_scheme.hpp 89.47% <0.00%> (-2.84%) ⬇️
include/seqan3/utility/views/single_pass_input.hpp 97.22% <0.00%> (-2.78%) ⬇️
include/seqan3/io/detail/in_file_iterator.hpp 94.28% <0.00%> (-2.69%) ⬇️
include/seqan3/io/views/detail/take_until_view.hpp 96.55% <0.00%> (-1.67%) ⬇️
include/seqan3/io/detail/misc_output.hpp 94.44% <0.00%> (-0.30%) ⬇️
include/seqan3/io/sam_file/detail/cigar.hpp 97.82% <0.00%> (-0.22%) ⬇️
... and 220 more

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 d9294eb...e4a3969. Read the comment docs.

@eseiler
Copy link
Member

eseiler commented Apr 1, 2022

We have to do either of both:

namespace seqan3::custom
{
// Specialise the seqan3::custom::argument_parsing data structure to enable parsing of std::errc.
template <>
struct sharg::custom::argument_parsing<std::errc>
{
    // ...
};

} // namespace seqan3::custom
namespace sharg::custom
{
// Specialise the sharg::custom::argument_parsing data structure to enable parsing of std::errc.
template <>
struct argument_parsing<std::errc>
{
    // ...
};

} // namespace sharg::custom

@smehringer
Copy link
Member Author

smehringer commented Apr 1, 2022

// Specialise the seqan3::custom::argument_parsing data structure to enable parsing of std::errc.
template <>
struct seqan3::custom::argument_parsing<std::errc>
{
    // ...
};

works and should be promoted

@vercel
Copy link

vercel bot commented Apr 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
seqan3 ❌ Failed (Inspect) Jun 3, 2022 at 10:33AM (UTC)

This overload would be needed anyway when
using foo with the seqan3::argument_parser.
It was not enforced by the validator itself but
is enforced in sharg. Since you don't use
validators without the argument_parser this
can be considered a fix not an API break.
This gets rid of dependency on the private test accessor
on seqan3::detail::format_help. When we use the sharg
parser, we do not have access to format_help anymore.
@seqan-actions seqan-actions added the lint [INTERNAL] signal for linting label Jun 2, 2022
@seqan-actions seqan-actions removed the lint [INTERNAL] signal for linting label Jun 2, 2022
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Jun 3, 2022
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Jun 3, 2022
@SGSSGene
Copy link
Contributor

We will include sharg as a submodule, but not integrate it into seqan3 namespace

@SGSSGene SGSSGene closed this Oct 18, 2022
@smehringer smehringer deleted the sharg branch November 27, 2023 06:59
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.

None yet

4 participants