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] Move core/common_tuple.hpp -> core/tuple/common_tuple.hpp #2273

Merged
merged 5 commits into from
Dec 16, 2020
Merged

[MISC] Move core/common_tuple.hpp -> core/tuple/common_tuple.hpp #2273

merged 5 commits into from
Dec 16, 2020

Conversation

eaasna
Copy link
Contributor

@eaasna eaasna commented Nov 18, 2020 •
edited
Loading

@smehringer
Copy link
Member

Nice! :) All tests pass 👍 You can now request seqan/team on the right hand side, such that the first review round starts.

@smehringer smehringer requested review from a team and MitraDarja and removed request for smehringer and a team November 23, 2020 07:15
@smehringer
Copy link
Member

Rene and I are always doing the second review :) Other team members are doing the first. You can literally request seqan/team and it will randomly select one of the first review team members. Just search for team in the review box next time.

MitraDarja
MitraDarja requested changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@MitraDarja MitraDarja left a comment

Choose a reason for hiding this comment

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

Looks good, well done! 👍
Just two small things then it can go to the second review. Just request me again, once you have added the changes.

include/seqan3/core/common_tuple.hpp Outdated
Comment on lines 10 to 11
* \deprecated This header is deprecated and will be removed in SeqAn-3.1.
* Please \#include <seqan3/core/tuple/common_tuple.hpp> instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* \deprecated This header is deprecated and will be removed in SeqAn-3.1.
* Please \#include <seqan3/core/tuple/common_tuple.hpp> instead.
* \deprecated This header will be removed in 3.1.0; Please \#include <seqan3/core/tuple/common_tuple.hpp> instead.

include/seqan3/core/common_tuple.hpp Outdated
using SEQAN3_DOXYGEN_ONLY(common_pair =) ::ranges::common_pair;

} // namespace seqan3
SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1. "
SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1.0. "

MitraDarja
MitraDarja requested changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@MitraDarja MitraDarja left a comment

Choose a reason for hiding this comment

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

Just two more things. There is a merge conflict now. Can you solve it? :)

include/seqan3/core/common_tuple.hpp Outdated

} // namespace seqan3
SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1.0 "
"Please #include <seqan3/core/tuple/common_tuple.hpp> instead.")
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a new line missing in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks for noticing :)

MitraDarja
MitraDarja approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@MitraDarja MitraDarja left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@MitraDarja MitraDarja requested review from a team and smehringer and removed request for a team November 25, 2020 16:10
@codecov
Copy link

codecov bot commented Nov 25, 2020 •
edited
Loading

Codecov Report

Merging #2273 (b37922b) into master (d37ba83) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2273      +/-   ##
==========================================
+ Coverage   98.15%   98.19%   +0.03%     
==========================================
  Files         262      262              
  Lines       10815    10853      +38     
==========================================
+ Hits        10616    10657      +41     
+ Misses        199      196       -3
Impacted Files Coverage Δ
include/seqan3/range/views/pairwise_combine.hpp 100.00% <ø> (ø)
include/seqan3/argument_parser/argument_parser.hpp 98.77% <0.00%> (-0.01%) ⬇️
include/seqan3/range/views/to_lower.hpp 100.00% <0.00%> (ø)
include/seqan3/range/views/to_upper.hpp 100.00% <0.00%> (ø)
include/seqan3/alphabet/quality/phred42.hpp 100.00% <0.00%> (ø)
include/seqan3/alphabet/quality/phred63.hpp 100.00% <0.00%> (ø)
include/seqan3/io/alignment_file/format_bam.hpp 95.58% <0.00%> (ø)
...clude/seqan3/alignment/pairwise/align_pairwise.hpp 100.00% <0.00%> (ø)
...de/seqan3/argument_parser/detail/version_check.hpp 92.91% <0.00%> (ø)
...qan3/alignment/pairwise/alignment_configurator.hpp 97.67% <0.00%> (ø)
... and 25 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 d37ba83...b37922b. Read the comment docs.

@marehr
Copy link
Member

marehr commented Dec 15, 2020

@smehringer Can you review this PR? It is waiting some days already :)

@smehringer
Copy link
Member

whoope I really thought I had reviewed that one. Thanks for the ping

smehringer
smehringer approved these changes Dec 15, 2020
View reviewed changes
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

only a tiny thing

include/seqan3/core/common_tuple.hpp Outdated Show resolved Hide resolved
@smehringer smehringer merged commit 7d28352 into seqan:master Dec 16, 2020
@eaasna eaasna deleted the moving_common_tuple branch December 21, 2020 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smehringer smehringer smehringer approved these changes

@MitraDarja MitraDarja MitraDarja 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
4 participants
@eaasna @smehringer @marehr @MitraDarja