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

[Alignment Configuration] Rename align_cfg::alignment_result_capture to align_cfg::detail::result_type #2031

Conversation

simonsasse
Copy link
Member

@simonsasse simonsasse commented Aug 13, 2020

Resolves seqan/product_backlog#183

This moves the alignment_result_capture to a new namespace seqan3::align_cfg::detail and then renames it to result_type.
I needed to specify the whole namespace path seqan3::detail::XX for most of the view files. This is due to the newly created seqan3::align_cfg::detail which had a name clash with seqan3::detail.

Merging this PR closes #2018 because it has the same intention.

Blocked by #2033

@simonsasse simonsasse force-pushed the feature/alignment_result_capture_renaming branch from ba6e6d5 to a9dff81 Compare August 13, 2020 11:59
@simonsasse simonsasse marked this pull request as ready for review August 13, 2020 12:03
@simonsasse simonsasse force-pushed the feature/alignment_result_capture_renaming branch 2 times, most recently from 0717a10 to 22167f8 Compare August 13, 2020 13:37
@@ -34,6 +32,7 @@ enum struct align_config_id : uint8_t
on_result, //!< ID for the \ref seqan3::align_cfg::on_result "on_result" option.
parallel, //!< ID for the \ref seqan3::align_cfg::parallel "parallel" option.
result, //!< ID for the \ref seqan3::align_cfg::result "result" option.
result_type, //!\brief ID for the \ref seqan3::align_cfg::detail::result_type "result_type" option.
Copy link
Member

Choose a reason for hiding this comment

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

This is different from the other lines... I don't know what's the impact, but please consider to keep the existing style.

Suggested change
result_type, //!\brief ID for the \ref seqan3::align_cfg::detail::result_type "result_type" option.
result_type, //!< ID for the \ref seqan3::align_cfg::detail::result_type "result_type" option.

Copy link
Member

Choose a reason for hiding this comment

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

Attention: Here is detail code documentation in the user visible documentation!
@ second reviewer: Is it fine or should we avoid this? (how?)

CHANGELOG.md Outdated Show resolved Hide resolved
@joergi-w
Copy link
Member

Merging this PR closes #2018 because it has the same intention.

Copy link
Member

@joergi-w joergi-w 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, just a small documentation error is left. I assign the second reviewer now.

include/seqan3/alignment/configuration/detail.hpp Outdated Show resolved Hide resolved
@joergi-w joergi-w requested review from a team and rrahn and removed request for a team August 14, 2020 10:16
@marehr
Copy link
Member

marehr commented Aug 14, 2020

@seqan/core We have now some redundant work #2020 they both worked around the ambiguous seqan3::detail, seqan3::align_cfg::detail namespace. I added PR #2033 to extract their common parts.

@rrahn
Copy link
Contributor

rrahn commented Aug 14, 2020

At the end I will have a final sweep to check for consistency issues that might happen between the PRs. I already had one config on the align_cfg::detail namespace and adapted only the things locally and wanted to add the namespace at the end. I am wondering why this was now exposed to all configs? Anyhow, now it is done already. We should wait for this change (already approved) and then rebase all the configs on top of this.

@rrahn
Copy link
Contributor

rrahn commented Aug 14, 2020

@simonsasse I am waiting on the rebase on #2020 before I review. So I can focus on the actual changes.

@simonsasse simonsasse force-pushed the feature/alignment_result_capture_renaming branch from 4aad14d to 66f3f01 Compare August 17, 2020 07:27
@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #2031 into release-3.0.2 will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release-3.0.2    #2031   +/-   ##
==============================================
  Coverage          97.91%   97.91%           
==============================================
  Files                263      263           
  Lines               9887     9887           
==============================================
  Hits                9681     9681           
  Misses               206      206           
Impacted Files Coverage Δ
...e/seqan3/alignment/pairwise/detail/type_traits.hpp 100.00% <ø> (ø)
...qan3/alignment/pairwise/alignment_configurator.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 c53fceb...686436d. Read the comment docs.

Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

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

Some general renaming, otherwise looks very good. Thank you

@simonsasse simonsasse force-pushed the feature/alignment_result_capture_renaming branch 2 times, most recently from 380538f to 8cb5d28 Compare August 20, 2020 08:25
@simonsasse
Copy link
Member Author

Thanks for the review @rrahn . I applied all the changes and also rebased on the latest version of the release branch because there where merge conflicts. On my machine tests pass 💻✅

@simonsasse simonsasse force-pushed the feature/alignment_result_capture_renaming branch from 8cb5d28 to 1efd0ca Compare August 20, 2020 08:31
@simonsasse simonsasse requested review from rrahn and a team and removed request for rrahn August 20, 2020 09:36
@marehr marehr removed the request for review from a team August 24, 2020 08:32
@marehr marehr self-requested a review August 24, 2020 08:32
@simonsasse simonsasse requested review from a team and removed request for a team August 24, 2020 08:47
@smehringer
Copy link
Member

whats the state here?

@simonsasse
Copy link
Member Author

If @marehr review counts as second review, its time for a merge I think :)

@smehringer
Copy link
Member

@marehr anything to add? Otherwise I'll merge (or you if you see this).

@marehr marehr merged commit 0eefbab into seqan:release-3.0.2 Aug 27, 2020
@marehr
Copy link
Member

marehr commented Aug 27, 2020

I only waited on CI and forgot that I waited in CI 🙈 (I'm not accustomed to be second reviewer) xD

@simonsasse simonsasse deleted the feature/alignment_result_capture_renaming branch March 24, 2021 11:26
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.

5 participants