-
Notifications
You must be signed in to change notification settings - Fork 82
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] Replace seqan3::views::join
with seqan3::views::join_with
#2526
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/AkersZk3zyum66KFxu6EKy9wWbQ4 |
75cbfec
to
6cc9cbb
Compare
Codecov Report
@@ Coverage Diff @@
## master #2526 +/- ##
=======================================
Coverage 98.24% 98.24%
=======================================
Files 270 270
Lines 10570 10570
=======================================
Hits 10384 10384
Misses 186 186
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one question. :)
Rename file to "join_with_view.hpp" |
6cc9cbb
to
0210902
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
#include <seqan3/range/views/repeat_n.hpp> | ||
#include <seqan3/range/views/slice.hpp> | ||
#include <seqan3/utility/views/join_with.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even used in this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was cold. Needed my computer to heat up ;-)
Yes, not needed at all, no clue why I added it.
#include <seqan3/utility/char_operations/predicate.hpp> | ||
#include <seqan3/utility/detail/type_name_as_string.hpp> | ||
#include <seqan3/utility/views/join_with.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even used in this class?
using ::ranges::views::join; | ||
|
||
} // namespace seqan3::views | ||
SEQAN3_DEPRECATED_HEADER( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: never mind, see second post
SEQAN3_DEPRECATED_HEADER( | |
namespace seqan3::views | |
{ | |
/*!\brief A join view. | |
* \ingroup views | |
* \deprecated Please use std::views::join or seqan3::views::join_with (if a separator is needed) | |
*/ | |
SEQAN3_DEPRECATED_310 inline constexpr auto join = ::ranges::views::join; | |
} // namespace seqan3::views | |
SEQAN3_DEPRECATED_HEADER( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, your initial solution might be better in this case.
- as we can deprecate
seqan3/range/views/join.hpp
and - don't use that internally any more
seqan3/range/views/all.hpp
includesseqan3/utility/views/join_with.hpp
and thus exposes the deprecatedseqan3::views::join
that means everything compiles as before, using all.hpp will not throw any warnings. an explicit include will give a proper deprecation warning.
0210902
to
dda55c7
Compare
@SGSSGene I'll rebase this for you :) |
- Deprecates `seqan3::views::join` use `seqan3::views::join_with` or `std::views::join` instead. - Adds `seqan3::views::join_with` - Move files from seqan3/ranges/views/join.hpp to seqan3/utility/views/join_with.hpp
dda55c7
to
e963591
Compare
seqan3::views::join
useseqan3::views::join_with
orstd::views::join
instead.seqan3::views::join_with
resolves seqan/product_backlog#351