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

[INFRA] Remove algorithm/bound.hpp & alignment/band/static_band.hpp #2272

Merged

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Nov 18, 2020

Resolves partly seqan/product_backlog#160

Removed because not used.

Details for this: https://gist.github.com/rrahn/f6b09ad67bebd14bd9c58f77c53e0450

@Irallia Irallia self-assigned this Nov 18, 2020
@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #2272 (047b1e3) into master (f66fe5b) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2272   +/-   ##
=======================================
  Coverage   98.17%   98.18%           
=======================================
  Files         262      260    -2     
  Lines       10813    10806    -7     
=======================================
- Hits        10616    10610    -6     
+ Misses        197      196    -1     

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 f66fe5b...047b1e3. Read the comment docs.

@Irallia Irallia force-pushed the INFRA/core/remove_bound_and_static_band branch 2 times, most recently from 9fb35be to bd1eee7 Compare November 18, 2020 12:44
@marehr
Copy link
Member

marehr commented Nov 18, 2020

@rrahn Why should this be deprecated? You can't do anything with that header anyway, shouldn't it just be removed? Since it is a left-over of the configuration change?

@Irallia Irallia requested review from a team and simonsasse and removed request for a team November 19, 2020 13:24
Copy link
Member

@simonsasse simonsasse 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 to me :)

@Irallia Irallia requested review from a team and smehringer and removed request for a team November 23, 2020 11:09
@smehringer
Copy link
Member

Since is something @rrahn requested I'll pass the review to him. I'll take on another PR

@smehringer smehringer requested review from rrahn and removed request for smehringer November 23, 2020 20:02
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.

Thank you. There are some general differences to just deprecating the headers, since the actual classes need to be deprecated.

@@ -26,9 +29,6 @@ namespace seqan3
class static_band
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the class should be deprecated.

Suggested change
class static_band
class SEQAN3_DEPRECATED_310 static_band

Copy link
Contributor

Choose a reason for hiding this comment

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

And then it would need an updated documentation that this class has been deprecated and one should use the align_cfg::band_fixed_size instead.

@@ -17,6 +18,8 @@
#include <seqan3/core/algorithm/bound.hpp>
#include <seqan3/std/concepts>

SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1.0.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed then, since the class itself gets deprecated

* \author Rene Rahn <rene.rahn AT fu-berlin.de>
* \deprecated This header will be removed in 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
* \deprecated This header will be removed in 3.1.
* \deprecated This header will be removed in 3.1.0; The contained functionality has been replaced by the seqan3::align_cfg::band_fixed_size configuration.

@@ -32,3 +33,5 @@
*/

#include <seqan3/alignment/band/static_band.hpp>

SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1.0.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same text as above.

*/

#pragma once

#include <seqan3/core/concept/core_language.hpp>
#include <seqan3/core/detail/strong_type.hpp>

SEQAN3_DEPRECATED_HEADER("This header is deprecated and will be removed in SeqAn-3.1.0.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well. We need the SEQAN3_DEPRECATED_310 macro before the strong type declarations and update the documentation of the classes with a deprecation notice.

@Irallia Irallia requested a review from rrahn November 25, 2020 15:38
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.

Thank you. One little thing though.

@@ -73,7 +73,7 @@
* \include test/snippet/core/algorithm/configuration_get_or.cpp
*/
#include <seqan3/core/algorithm/algorithm_result_generator_range.hpp>
#include <seqan3/core/algorithm/bound.hpp>
#include <seqan3/core/algorithm/bound.hpp> // ToDo: remove this with the release SeqAn-3.1.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the macros to issue a warning message when the versions have been updated so we can't forget about this.

@Irallia Irallia force-pushed the INFRA/core/remove_bound_and_static_band branch from a6aaea1 to 6cccfdb Compare November 30, 2020 12:55
@Irallia Irallia requested a review from rrahn November 30, 2020 15:01
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.

thank you

@rrahn
Copy link
Contributor

rrahn commented Dec 5, 2020

Needs a rebase against current master. Thank you!

@Irallia Irallia force-pushed the INFRA/core/remove_bound_and_static_band branch from 6cccfdb to 047b1e3 Compare December 7, 2020 12:41
@smehringer smehringer merged commit c08c21f into seqan:master Dec 7, 2020
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