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

Restructure stream handles from files to formats. #1156

Closed
wants to merge 1 commit into from
Closed

Restructure stream handles from files to formats. #1156

wants to merge 1 commit into from

Conversation

smehringer
Copy link
Member

@smehringer smehringer commented Jul 4, 2019 •
edited
Loading

Blocked by #1160
Blocked by #1184

Moves the stream handle from the file to the format
and for alignment SAM removes all usage of operator<<() in favour of working directly on the ostreambuf_iterator.

@smehringer smehringer requested a review from eseiler July 4, 2019 12:33
eseiler
eseiler requested changes Jul 4, 2019
View reviewed changes
include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved
include/seqan3/io/alignment_file/format_bam.hpp Show resolved Hide resolved
include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved
include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved
include/seqan3/io/alignment_file/input.hpp Outdated Show resolved Hide resolved
test/unit/io/alignment_file/alignment_file_format_test_template.hpp Outdated Show resolved Hide resolved
test/unit/io/alignment_file/alignment_file_output_test.cpp Outdated Show resolved Hide resolved
test/unit/io/alignment_file/format_bam_test.cpp Outdated Show resolved Hide resolved
include/seqan3/std/charconv Outdated Show resolved Hide resolved
test/unit/std/charconv_test.cpp Outdated Show resolved Hide resolved
@smehringer smehringer force-pushed the alignment_file_in branch from 454a307 to df55d5a Compare July 8, 2019 13:11
@seqan seqan deleted a comment from eseiler Jul 9, 2019
@smehringer smehringer force-pushed the alignment_file_in branch from df55d5a to 50d1e48 Compare July 9, 2019 10:23
@codecov
Copy link

codecov bot commented Jul 9, 2019 •
edited
Loading

Codecov Report

Merging #1156 into master will decrease coverage by 0.17%.
The diff coverage is 97.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1156      +/-   ##
==========================================
- Coverage   96.86%   96.69%   -0.18%     
==========================================
  Files         212      209       -3     
  Lines        8274     8058     -216     
==========================================
- Hits         8015     7792     -223     
- Misses        259      266       +7
Impacted Files Coverage Δ
include/seqan3/io/detail/misc.hpp 100% <100%> (ø) ⬆️
include/seqan3/io/alignment_file/output.hpp 100% <100%> (ø) ⬆️
include/seqan3/io/sequence_file/format_fasta.hpp 97.61% <100%> (+0.35%) ⬆️
include/seqan3/io/structure_file/input.hpp 100% <100%> (+1.63%) ⬆️
include/seqan3/io/sequence_file/format_embl.hpp 96.51% <100%> (-0.93%) ⬇️
include/seqan3/io/detail/misc_input.hpp 95.74% <100%> (-0.49%) ⬇️
include/seqan3/io/alignment_file/format_sam.hpp 99.02% <100%> (+0.04%) ⬆️
include/seqan3/io/alignment_file/input.hpp 100% <100%> (ø) ⬆️
include/seqan3/io/structure_file/output.hpp 100% <100%> (ø) ⬆️
include/seqan3/io/sequence_file/output.hpp 100% <100%> (ø) ⬆️
... and 32 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 5a0354a...6307805. Read the comment docs.

@smehringer smehringer force-pushed the alignment_file_in branch 2 times, most recently from 27a0087 to 7882931 Compare July 9, 2019 15:23
@smehringer smehringer requested a review from eseiler July 10, 2019 06:16
eseiler
eseiler requested changes Jul 10, 2019
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.

Just a few minor comments.

But, theoretically, read and write are public (e.g. here). So there should be some deprecation? 🤐

include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved
include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved
include/seqan3/io/alignment_file/format_bam.hpp Outdated Show resolved Hide resolved
include/seqan3/io/alignment_file/format_bam.hpp Show resolved Hide resolved
include/seqan3/io/alignment_file/format_sam.hpp Show resolved Hide resolved
include/seqan3/io/sequence_file/format_embl.hpp Show resolved Hide resolved
@smehringer
Copy link
Member Author

But, theoretically, read and write are public (e.g. here). So there should be some deprecation? zipper_mouth_face

no they are not! we do not use the formats but the files for reading :)

@smehringer smehringer changed the title 2 changes in one: restructure stream handles, to_chars overload Restructure stream handles from files to formats. Jul 11, 2019
@smehringer smehringer requested a review from eseiler July 11, 2019 13:06
@eseiler
Copy link
Member

eseiler commented Jul 11, 2019

But, theoretically, read and write are public (e.g. here). So there should be some deprecation? zipper_mouth_face

no they are not! we do not use the formats but the files for reading :)

Yes I know, but my point is that you can find it in the user docs ... i.e. if someone wanted to use it, they could and it would be a public api.
(Not that I want it to be deprecated.)

@smehringer
Copy link
Member Author

Yes I know, but my point is that you can find it in the user docs.

ahh. true. Darn, I am not sure if this should stay public..
@h-2 the read functions of the detail format is public through the public input/output format concept :(

@eseiler
Copy link
Member

eseiler commented Jul 11, 2019 •
edited
Loading

Yes I know, but my point is that you can find it in the user docs.

ahh. true. Darn, I am not sure if this should stay public..
@h-2 the read functions of the detail format is public through the public input/output format concept :(

We can mark them with \noapi or whatever it is called.
I don't think it should be public since we want users to use the files.

@h-2
Copy link
Member

h-2 commented Jul 11, 2019

Remember that we do want people to be able to add their own formats so there need to be public concepts that define the interface of such formats.

@smehringer
Copy link
Member Author

smehringer commented Jul 11, 2019 •
edited
Loading

But currently you cannot add your own format unless in the seqan3 namespace 🙈

@h-2
Copy link
Member

h-2 commented Jul 11, 2019

Yes, but we explicitly said before the release that this was something we want to fix before 3.1

@smehringer
Copy link
Member Author

Yes, was there a solution already?

@smehringer smehringer requested a review from h-2 August 27, 2019 09:00
@h-2
Copy link
Member

h-2 commented Aug 30, 2019

So we delay this for now?

@h-2 h-2 removed their request for review August 30, 2019 10:56
@smehringer
Copy link
Member Author

So we delay this for now?

Yes.

@marehr
Copy link
Member

marehr commented Sep 30, 2019

blocked by other restructurings, close for now

@marehr marehr closed this Sep 30, 2019
@smehringer smehringer deleted the alignment_file_in branch November 4, 2020 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
4 participants
@smehringer @eseiler @h-2 @marehr