[PATCH] deprecate core_language.hpp #2351

SGSSGene · 2021-01-25T13:33:43Z

Move content of core/concept/core_language.hpp to utility/detail/exposition_only_concept.hpp
Added deprecation to core_language.hpp and a forward include to exposition_only_concept.hpp
Changed all header files to new include path
Changed all unittest to new include path

Resolves part of seqan/product_backlog#160

SGSSGene · 2021-01-25T13:34:06Z

This is solving partly seqan/product_backlog#160

codecov · 2021-01-25T14:03:28Z

Codecov Report

Merging #2351 (7b01ba2) into master (94a4e22) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2351   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files         265      265           
  Lines       10806    10806           
=======================================
  Hits        10613    10613           
  Misses        193      193

Impacted Files	Coverage Δ
...lude/seqan3/alignment/matrix/alignment_optimum.hpp	`100.00% <ø> (ø)`
...qan3/alignment/matrix/detail/affine_cell_proxy.hpp	`100.00% <ø> (ø)`
...atrix/detail/alignment_score_matrix_one_column.hpp	`100.00% <ø> (ø)`
.../detail/alignment_score_matrix_one_column_base.hpp	`100.00% <ø> (ø)`
.../matrix/detail/combined_score_and_trace_matrix.hpp	`100.00% <ø> (ø)`
...qan3/alignment/matrix/detail/matrix_coordinate.hpp	`100.00% <ø> (ø)`
...nment/matrix/detail/score_matrix_single_column.hpp	`100.00% <ø> (ø)`
...qan3/alignment/matrix/detail/trace_matrix_full.hpp	`100.00% <ø> (ø)`
...ment/pairwise/detail/alignment_algorithm_state.hpp	`100.00% <ø> (ø)`
...n3/alignment/pairwise/policy/affine_gap_policy.hpp	`100.00% <ø> (ø)`
... and 17 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 94a4e22...7b01ba2. Read the comment docs.

Irallia

Thank you!
Could you please move the belonging tests aswell?

Irallia · 2021-01-26T12:05:50Z

test/unit/std/concept/core_language_test.cpp

 #include <random>

-#include <seqan3/core/concept/core_language.hpp>
+#include <seqan3/utility/detail/exposition_only_concept.hpp>
 #include <seqan3/std/iterator>

 #include "auxiliary.hpp"


Could you also move and rename this test?
test/unit/std/concept/core_language_test.cpp -> test/unit/utility/detail/exposition_only_concept_test.cpp
And double check if there are snipptes or performance tests to move?

include/seqan3/utility/detail/exposition_only_concept.hpp

marehr · 2021-02-01T10:37:48Z

2021-02-01 Core Meeting

We talked about documentation inconsistencies.

We found that the documentation of seqan3::detail::template_specialisation_of is wrong

Remove addtogroup
Fix namespace of seqan3::template_specialisation_of -> seqan3::detail::template_specialisation_of, see here

Move concept group from core to utility. Keep seqan3::implicitly_convertible_to and so on public, but explicitly mark them as \noapi.

SGSSGene · 2021-02-05T13:21:40Z

Some question came up:

where to put the \defgroup utility_concept Concept. Everywhere else it is inside an all.hpp, but I don't really have one. I have put it for now into utility/all.hpp
core/all.hpp had an unuseful concept description. I for now removed it.

Irallia

Thanks, looks good now!
I think you can remove the include from the all.hpp, but you can do this together with the second review.

Irallia · 2021-02-08T10:59:45Z

include/seqan3/core/concept/all.hpp

@@ -13,14 +13,5 @@
 #pragma once

 #include <seqan3/core/concept/cereal.hpp>
-#include <seqan3/core/concept/core_language.hpp>
+#include <seqan3/utility/detail/exposition_only_concept.hpp>


So far I know, we don't put detail things into all.hpp files.
@seqan/core is that correct?

Some question came up:

1. where to put the `\defgroup utility_concept Concept`. Everywhere else it is inside an all.hpp, but I don't really have one. I have put it for now into `utility/all.hpp` 2. core/all.hpp had an unuseful `concept` description. I for now removed it.

For detail folder there is no all.hpp.
We actually want to establish to never just include 'all.hpp' but only what you need. The 'all.hpp' are for the user, who is not so familiar with the different modules etc.
Things in detail are functionalities that are there for us and not for the user, so there is no 'all.hpp'.

So far I know, we don't put detail things into all.hpp files.
@seqan/core is that correct?

In general, you are right, but I think it is okay in this case. In the long-run we will remove that header.

marehr

Looks very good; just some (repetetive) remarks :)

include/seqan3/alignment/matrix/alignment_optimum.hpp

include/seqan3/alignment/matrix/detail/affine_cell_proxy.hpp

include/seqan3/alignment/matrix/detail/alignment_score_matrix_one_column_base.hpp

include/seqan3/alignment/matrix/detail/combined_score_and_trace_matrix.hpp

include/seqan3/alignment/matrix/detail/matrix_coordinate.hpp

test/unit/std/concept/comparison_test.cpp

test/unit/std/concept/object_test.cpp

test/unit/utility/detail/exposition_only_concept_test.cpp

- Move content of core/concept/core_language.hpp to utility/detail/exposition_only_concept.hpp - Added deprecation to core_language.hpp and a forward include to exposition_only_concept.hpp - Changed all header files to new include path - Changed all unittest to new include path

• remove from `concept` group • add missing `detail::` namespace scope

Co-authored-by: Marcel <marehr@users.noreply.github.com>

marehr

Thank you so much, it seems we are done. :)

SGSSGene mentioned this pull request Jan 25, 2021

Core / Utility Split seqan/product_backlog#160

Closed

23 tasks

SGSSGene requested review from a team and Irallia and removed request for a team January 25, 2021 13:35

Irallia requested changes Jan 26, 2021

View reviewed changes

SGSSGene force-pushed the patch/deprecate-core_language branch from 950d403 to e1bdf68 Compare February 5, 2021 13:16

SGSSGene force-pushed the patch/deprecate-core_language branch from e1bdf68 to fccb332 Compare February 5, 2021 13:24

marehr requested a review from Irallia February 8, 2021 09:09

Irallia approved these changes Feb 8, 2021

View reviewed changes

Irallia requested a review from marehr February 8, 2021 11:03

marehr reviewed Feb 12, 2021

View reviewed changes

SGSSGene force-pushed the patch/deprecate-core_language branch from e2fac71 to 08d176b Compare February 14, 2021 16:55

SGSSGene and others added 6 commits February 14, 2021 18:36

[FIX] rename unittest to utility/detail/exposition_only_concept_test.cpp

5dbb7c7

[FIX] doxygen - adjusting template_specicialisation_of

a93d692

• remove from `concept` group • add missing `detail::` namespace scope

[FIX] introducing to doxygen

c6bc2ea

[FIX] reorder of includes

23a21b9

Co-authored-by: Marcel <marehr@users.noreply.github.com>

[FIX] more reordering of includes

7b01ba2

SGSSGene force-pushed the patch/deprecate-core_language branch from 08d176b to 7b01ba2 Compare February 14, 2021 18:04

SGSSGene mentioned this pull request Feb 14, 2021

[INFRA] adds a helper script to check for missing includes #2376

Merged

marehr self-requested a review February 15, 2021 09:11

marehr approved these changes Feb 15, 2021

View reviewed changes

marehr merged commit a1d5d4d into seqan:master Feb 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH] deprecate core_language.hpp #2351

[PATCH] deprecate core_language.hpp #2351

SGSSGene commented Jan 25, 2021 •

edited by marehr

SGSSGene commented Jan 25, 2021

codecov bot commented Jan 25, 2021 •

edited

Irallia left a comment

Irallia Jan 26, 2021

marehr commented Feb 1, 2021 •

edited

SGSSGene commented Feb 5, 2021

Irallia left a comment

Irallia Feb 8, 2021

Irallia Feb 8, 2021

marehr Feb 15, 2021

marehr left a comment

marehr left a comment

[PATCH] deprecate core_language.hpp #2351

[PATCH] deprecate core_language.hpp #2351

Conversation

SGSSGene commented Jan 25, 2021 • edited by marehr

SGSSGene commented Jan 25, 2021

codecov bot commented Jan 25, 2021 • edited

Codecov Report

Irallia left a comment

Choose a reason for hiding this comment

Irallia Jan 26, 2021

Choose a reason for hiding this comment

marehr commented Feb 1, 2021 • edited

2021-02-01 Core Meeting

SGSSGene commented Feb 5, 2021

Irallia left a comment

Choose a reason for hiding this comment

Irallia Feb 8, 2021

Choose a reason for hiding this comment

Irallia Feb 8, 2021

Choose a reason for hiding this comment

marehr Feb 15, 2021

Choose a reason for hiding this comment

marehr left a comment

Choose a reason for hiding this comment

marehr left a comment

Choose a reason for hiding this comment

SGSSGene commented Jan 25, 2021 •

edited by marehr

codecov bot commented Jan 25, 2021 •

edited

marehr commented Feb 1, 2021 •

edited