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] deprecate seqan3::alignment_file* entities #2459

Merged
merged 2 commits into from
Mar 23, 2021
Merged

[MISC] deprecate seqan3::alignment_file* entities #2459

merged 2 commits into from
Mar 23, 2021

Conversation

marehr
Copy link
Member

@marehr marehr commented Mar 21, 2021

@marehr marehr requested review from a team and joergi-w and removed request for a team March 21, 2021 18:38
@vercel
Copy link

vercel bot commented Mar 21, 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/6TJXN2hpc9fmRop8kWcCMeMSEwqX
✅ Preview: https://seqan3-git-fork-marehr-samfiledeprecation-seqan.vercel.app

@marehr marehr force-pushed the sam_file_deprecation branch 2 times, most recently from 7baef2f to 118b163 Compare March 21, 2021 18:46
@codecov
Copy link

codecov bot commented Mar 21, 2021 •
edited
Loading

Codecov Report

Merging #2459 (6de1605) into master (de4db1f) will not change coverage.
The diff coverage is n/a.

❗ Current head 6de1605 differs from pull request most recent head d79db50. Consider uploading reports for the commit d79db50 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2459   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files         267      267           
  Lines       11063    11063           
=======================================
  Hits        10875    10875           
  Misses        188      188
Impacted Files Coverage Δ
include/seqan3/argument_parser/argument_parser.hpp 98.77% <ø> (ø)
include/seqan3/argument_parser/validators.hpp 92.30% <ø> (ø)
include/seqan3/io/sam_file/output.hpp 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 de4db1f...d79db50. Read the comment docs.

joergi-w
joergi-w approved these changes Mar 22, 2021
View reviewed changes
include/seqan3/io/alignment_file/all.hpp
Comment on lines +18 to +27
#include <seqan3/io/alignment_file/format_bam.hpp>
#include <seqan3/io/alignment_file/format_sam.hpp>
#include <seqan3/io/alignment_file/header.hpp>
#include <seqan3/io/alignment_file/input.hpp>
#include <seqan3/io/alignment_file/input_format_concept.hpp>
#include <seqan3/io/alignment_file/input_options.hpp>
#include <seqan3/io/alignment_file/output.hpp>
#include <seqan3/io/alignment_file/output_format_concept.hpp>
#include <seqan3/io/alignment_file/output_options.hpp>
#include <seqan3/io/alignment_file/sam_tag_dictionary.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Are these includes still needed after #include <seqan3/io/sam_file/all.hpp>?

Copy link
Member Author

Choose a reason for hiding this comment

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

all.hpp won't completely work, because some headers like #include <seqan3/io/alignment_file/input.hpp> define the deprecated seqan3::alignment_file_input.

So to be safe I just used the old include list without changing anything.

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean there will be 11 deprecation warnings when including seqan3/io/alignment_file/all.hpp? 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, xD

@joergi-w joergi-w requested a review from eseiler March 22, 2021 08:58
eseiler
eseiler approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@eseiler eseiler left a comment

Choose a reason for hiding this comment

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

alignment_file_output_format and alignment_file_input_format do not show up in the documentation.

If you want to have a look, go ahead, otherwise you can merge

include/seqan3/io/alignment_file/all.hpp
Comment on lines +18 to +27
#include <seqan3/io/alignment_file/format_bam.hpp>
#include <seqan3/io/alignment_file/format_sam.hpp>
#include <seqan3/io/alignment_file/header.hpp>
#include <seqan3/io/alignment_file/input.hpp>
#include <seqan3/io/alignment_file/input_format_concept.hpp>
#include <seqan3/io/alignment_file/input_options.hpp>
#include <seqan3/io/alignment_file/output.hpp>
#include <seqan3/io/alignment_file/output_format_concept.hpp>
#include <seqan3/io/alignment_file/output_options.hpp>
#include <seqan3/io/alignment_file/sam_tag_dictionary.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean there will be 11 deprecation warnings when including seqan3/io/alignment_file/all.hpp? 😁

eseiler
eseiler approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@eseiler eseiler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

duplicate post due to server error

@marehr marehr merged commit 7974b77 into seqan:master Mar 23, 2021
@marehr marehr deleted the sam_file_deprecation branch March 23, 2021 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joergi-w joergi-w joergi-w approved these changes

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

None yet
3 participants
@marehr @joergi-w @eseiler