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

Deprecate/take exactly #2601

Merged
merged 2 commits into from
May 11, 2021
Merged

Deprecate/take exactly #2601

merged 2 commits into from
May 11, 2021

Conversation

SGSSGene
Copy link
Contributor

@SGSSGene SGSSGene commented May 4, 2021

@vercel
Copy link

vercel bot commented May 4, 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/5JHdbcAAUHYSNNQBHAR4nyX2ieW9
✅ Preview: https://seqan3-git-fork-sgssgene-deprecate-takeexactly-seqan.vercel.app

@SGSSGene SGSSGene force-pushed the deprecate/take_exactly branch 2 times, most recently from dd77c10 to a3e2ac7 Compare May 4, 2021 18:36
@codecov
Copy link

codecov bot commented May 4, 2021 •
edited
Loading

Codecov Report

Merging #2601 (9ed6ba0) into master (77c5e8a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2601   +/-   ##
=======================================
  Coverage   98.27%   98.27%           
=======================================
  Files         273      273           
  Lines       10795    10795           
=======================================
  Hits        10609    10609           
  Misses        186      186
Impacted Files Coverage Δ
include/seqan3/io/detail/take_view.hpp 98.33% <ø> (ø)
include/seqan3/io/sequence_file/format_fasta.hpp 98.68% <ø> (ø)
include/seqan3/io/sam_file/format_bam.hpp 96.29% <100.00%> (ø)
include/seqan3/io/sam_file/format_sam.hpp 97.44% <100.00%> (ø)
include/seqan3/io/sequence_file/format_fastq.hpp 100.00% <100.00%> (ø)
include/seqan3/utility/views/repeat_n.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 77c5e8a...9ed6ba0. Read the comment docs.

@marehr marehr requested review from a team and MitraDarja and removed request for a team May 4, 2021 23:19
@SGSSGene SGSSGene requested review from a team and removed request for a team May 5, 2021 07:45
@MitraDarja MitraDarja requested review from a team and eseiler and removed request for a team May 5, 2021 08:27
eseiler
eseiler requested changes May 5, 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.

The idea was that we do not just deprecate it but move it to detail because we need it internally, correct?
If so, can you do it like we have done it with the persist view? In short, we moved the view itself into detail and did the deprecation of the old header and the old view a bit differently

CHANGELOG.md Outdated
@@ -238,6 +238,8 @@ regression test suite and patches at https://github.com/seqan/seqan3/tree/master
[\#2541](https://github.com/seqan/seqan3/pull/2541).
* We deprecated `seqan3::views::take_line` and it will be removed in 3.1.0
[\#2525](https://github.com/seqan/seqan3/pull/2525).
* We deprecated `seqan3::views::take_exactly`. Please use `std::views::take` or `std::views::counted` instead
[\#2601](https://github.com/seqan/seqan3/pull/2601).
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
[\#2601](https://github.com/seqan/seqan3/pull/2601).
([\#2601](https://github.com/seqan/seqan3/pull/2601)).

@SGSSGene SGSSGene requested a review from eseiler May 10, 2021 08:43
@SGSSGene
Copy link
Contributor Author

The idea was that we do not just deprecate it but move it to detail because we need it internally, correct?
If so, can you do it like we have done it with the persist view? In short, we moved the view itself into detail and did the deprecation of the old header and the old view a bit differently

Yes, you are right. I changed it accordingly.

eseiler
eseiler requested changes May 10, 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.

Only minor stuff; can you rebase?

include/seqan3/io/detail/take_view.hpp Outdated Show resolved Hide resolved
include/seqan3/range/views/all.hpp Outdated Show resolved Hide resolved
include/seqan3/range/views/all.hpp Outdated Show resolved Hide resolved
include/seqan3/range/views/all.hpp Outdated Show resolved Hide resolved
test/snippet/io/detail/take_exactly.cpp Outdated Show resolved Hide resolved
@SGSSGene SGSSGene requested a review from eseiler May 11, 2021 07:18
@eseiler eseiler merged commit a0bdda4 into seqan:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

deprecate seqan3::views::take_exactly
3 participants
@SGSSGene @eseiler @MitraDarja