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

[SEARCH] Refactor search mode config to individual config elements #1639

Merged
merged 1 commit into from
May 19, 2020

Conversation

smehringer
Copy link
Member

@smehringer smehringer commented Mar 10, 2020

The search modes were renamed and can now be used as individual config elements.
Since this is orthogonal to the non-dynamic config element search_cfg::mode, mode was removed here. A follow-up PR will introduce the search_cfg::mode config element again as a dynamic selector.

Resolves seqan/product_backlog#65

@smehringer smehringer requested a review from eseiler March 10, 2020 14:56
CHANGELOG.md Outdated Show resolved Hide resolved
include/seqan3/search/configuration/all.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/all.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/all.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/all.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/all.hpp Outdated Show resolved Hide resolved
test/snippet/search/configuration_modes.cpp Outdated Show resolved Hide resolved
test/snippet/search/configuration_modes.cpp Outdated Show resolved Hide resolved
test/unit/search/search_configuration_test.cpp Outdated Show resolved Hide resolved
@smehringer smehringer requested a review from eseiler March 11, 2020 09:06
@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #1639 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1639   +/-   ##
=======================================
  Coverage   97.52%   97.52%           
=======================================
  Files         248      248           
  Lines        9436     9436           
=======================================
  Hits         9202     9202           
  Misses        234      234           
Impacted Files Coverage Δ
...ude/seqan3/search/detail/policy_result_builder.hpp 100.00% <ø> (ø)
...e/seqan3/search/detail/search_scheme_algorithm.hpp 94.50% <100.00%> (ø)
.../search/detail/unidirectional_search_algorithm.hpp 100.00% <100.00%> (ø)
include/seqan3/search/search.hpp 88.63% <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 c574e0c...5264f6c. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
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 minor stuff only and a general naming thing. Otherwise looks good to me.

include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
test/snippet/search/configuration_modes.cpp Outdated Show resolved Hide resolved
test/snippet/search/configuration_modes.cpp Outdated Show resolved Hide resolved
test/unit/search/search_test.cpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
@smehringer
Copy link
Member Author

Since I changed a lot after the design decision, @eseiler Can you review again? It feels like cheeting if I take the approval as is.

@smehringer smehringer requested a review from eseiler May 15, 2020 09:41
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.

Looks in general fine. Some minor changes to avoid copy'n'paste errors (I am not sure if you can copy the brief from the detail though, but at least the other way around should work.)
And I would try to be sparse with the word mode as it can be basically anything. So there are some suggestions to use another term.

include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
using detail::strong_type<uint8_t, strata, detail::strong_type_skill::convert>::strong_type;
//!\privatesection
//!\brief Internal id to check for consistent configuration settings.
static constexpr detail::search_config_id id{detail::search_config_id::mode};
};

/*!\brief Configuration element to determine the search mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the documentation.

include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
include/seqan3/search/configuration/mode.hpp Outdated Show resolved Hide resolved
@smehringer
Copy link
Member Author

I somehow forgot the dynamic selector will be called hit like the prefix and not mode. That's why I left all the notions of mode in there as it would connect to the selector. I will go over the documentation again and remove all notions of mode and try to replace it by strategy.

@smehringer smehringer requested a review from rrahn May 18, 2020 10:54
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.

lgtm, thanks

@rrahn
Copy link
Contributor

rrahn commented May 19, 2020

looks like you forgot to update the search configuration cmakelists after renaming the test files:

CMake Error at /home/travis/build/seqan/seqan3/test/coverage/CMakeLists.txt:84 (add_executable):

  Cannot find source file:

    mode_test.cpp

  Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp

  .hxx .in .txx

Call Stack (most recent call first):

  /home/travis/build/seqan/seqan3/test/unit/search/configuration/CMakeLists.txt:1 (seqan3_test)

@smehringer smehringer merged commit 1edd24f into seqan:master May 19, 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.

search configuration hit part 1
3 participants