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

[MISC] Deprecate views::take_until #2604

Merged
merged 3 commits into from
May 11, 2021
Merged

Conversation

SGSSGene
Copy link
Contributor

@SGSSGene SGSSGene commented May 5, 2021

Fixing issue seqan/product_backlog#332

  • seqan3::views::take_until is moved to seqan3/io/detail for
    internal use only. As an alternative please use
    std::views::take_while in combination with std::not_fn.
    or if using the seqan3::views::take_until_and_consume_* version add an extra std::views::drop_while to reproduce the same results.
  • moved snippets and test accordingly.
  • add CHANGLELOG entry

@vercel
Copy link

vercel bot commented May 5, 2021

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/HBKjDQPKx2hJuNw6mX7spyM2QsNd
✅ Preview: https://seqan3-git-fork-sgssgene-deprecate-takeuntil-seqan.vercel.app

[Deployment for 038edae failed]

@SGSSGene
Copy link
Contributor Author

SGSSGene commented May 5, 2021

What is currently missing is a more elaborated deprecation warning on how to replace take_until_*_constume with std::views::take_while

@SGSSGene SGSSGene requested review from a team, marehr and Irallia and removed request for a team and marehr May 5, 2021 08:51
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #2604 (038edae) into master (adc0324) will not change coverage.
The diff coverage is 98.93%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2604   +/-   ##
=======================================
  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% <ø> (ø)
...lude/seqan3/io/sam_file/detail/format_sam_base.hpp 97.82% <ø> (ø)
include/seqan3/io/detail/take_until_view.hpp 98.18% <98.18%> (ø)
include/seqan3/alphabet/views/trim_quality.hpp 100.00% <100.00%> (ø)
include/seqan3/io/sam_file/detail/cigar.hpp 97.75% <100.00%> (ø)
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_embl.hpp 100.00% <100.00%> (ø)
include/seqan3/io/sequence_file/format_fasta.hpp 98.68% <100.00%> (ø)
include/seqan3/io/sequence_file/format_fastq.hpp 100.00% <100.00%> (ø)
... and 4 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 a0bdda4...038edae. Read the comment docs.

Copy link
Contributor

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

It is possible that one of the includes is not needed, since I could not find a take_until in the file.

include/seqan3/io/sam_file/format_bam.hpp Outdated Show resolved Hide resolved
@SGSSGene SGSSGene changed the title [MISC] Dprecate views::take_until [MISC] Deprecate views::take_until May 5, 2021
@SGSSGene
Copy link
Contributor Author

SGSSGene commented May 5, 2021

@Irallia I have update the deprecation message in take_until_view.hpp for the 4 instances take_until, take_until_and_consume and the *_throw_* stuff. Can you take a very close look that it is understandable? thank you :-)

Copy link
Contributor

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

Thanks for the tip, I took a look at it.
I'm really not good at formulating nice English sentences, but I may have a few small improvements.

include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
* as their non-consuming counter-parts if the underlying range models at least `std::forward_range`.
* If, however, the underlying range is a pure `std::input_range`, the view will keep moving the underlying
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
* as their non-consuming counter-parts if the underlying range models at least `std::forward_range`.
* If, however, the underlying range is a pure `std::input_range`, the view will keep moving the underlying
* as their non-consuming counter-parts if the underlying range models at least `std::ranges::forward_range`.
* If, however, the underlying range is a pure `std::ranges::input_range`, the view will keep moving the underlying

Copy link
Contributor

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

One review you did not answer and one more I found. Also, the comment from marcel is still open (you changed it to std::range:: but it should be std::ranges::).

include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@Irallia Irallia left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@Irallia Irallia requested review from a team and eseiler and removed request for a team May 10, 2021 10:13
CHANGELOG.md Outdated
Comment on lines 248 to 249
* We deprecated `seqan3::views::take_until` and it will be removed in 3.1.0. Use `std::views::take_while` in
combination with `std::not_fn` ([\#2604](https://github.com/seqan/seqan3/pull/2604)).
Copy link
Member

Choose a reason for hiding this comment

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

I would have just said "with an inverted predicated/condition"

Copy link
Member

Choose a reason for hiding this comment

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

Or just "Use std::views::take_while(std::not(predicate)) instead" (I'd prefer this).

I think it's concise enough for a changelog, and it's more detailed in the documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, much better, thx!

Comment on lines 670 to 671
* \deprecated A previous call `view | take_until_and_consume(is_space)` could be convert to
* `view | std::take_while(std::not_fn(is_space)) | std::drop_while(is_space)`
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
* \deprecated A previous call `view | take_until_and_consume(is_space)` could be convert to
* `view | std::take_while(std::not_fn(is_space)) | std::drop_while(is_space)`
* \deprecated Use `std::take_while(std::not_fn(functor)) | std::drop_while(functor)` instead of
* `seqan3::views::take_until_and_consume(functor)`.

For me, this would be more helpful, because it reuses the functor which is mentioned in the brief, and technically you don't need the view |.

Could you adjust the other ones?
take_until should also get the "long" deprecation documentation

Copy link
Member

@marehr marehr May 10, 2021

Choose a reason for hiding this comment

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

Is this correct though?

I thought this does the stuff in sequence. First take everything until the first space and after that skip all spaces.

Right now it would be for an element: first take (filter) everything until first space and from that resulting range drop until the first space. That means you should get an empty range.

Copy link
Member

Choose a reason for hiding this comment

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

Hah, right. Is there even an easy replacement?

Copy link
Member

Choose a reason for hiding this comment

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

No there isn't if it isn't an input iterator.
I would just write that we don't have a replacement for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I remember Marcel and I already talked about this not being an appropriate replacement.
I now replaced the deprecation warning with "No alternative" as it was done in "as_const.hpp".

include/seqan3/range/views/all.hpp Show resolved Hide resolved
- `seqan3::views::take_until` is moved to `seqan3/io/detail` for
  internal use only. As an alternative please use
  `std::views::take_while` in combination with `std::not_fn`.
- moved snippets and test accordingly.
- add CHANGLELOG entry
include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
include/seqan3/io/detail/take_until_view.hpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
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