-
Notifications
You must be signed in to change notification settings - Fork 80
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
Cigar operation #2388
Cigar operation #2388
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/2LEeQHabL75AhAq6enW8CKhh7UZX |
9ecaa52
to
e94b2bc
Compare
include/seqan3/alphabet/cigar/exposition_only/cigar_operation.hpp
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2388 +/- ##
=======================================
Coverage 98.24% 98.24%
=======================================
Files 266 266
Lines 10928 10928
=======================================
Hits 10736 10736
Misses 192 192
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One really minor thing, otherwise it lgtm. :)
CHANGELOG.md
Outdated
@@ -88,8 +88,13 @@ If possible, provide tooling that performs the changes, e.g. a shell-script. | |||
|
|||
#### Alphabet | |||
|
|||
* Removed seqan3::char_is_valid_for requirement from seqan3::writable_alphabet and seqan3::writable_constexpr_alphabet | |||
* Removed seqan3::char_is_valid_for requirement from seqan3::detail::writable_alphabet and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks like it would belong to a different PR? Or did you just fix this because you touched the file already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just corrected a changelog entry (but the wrong one 🙈), if you want, I can create a new PR to fix it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that is not necessary. Just wanted to make sure, that this was on purpose and not an accident. ;)
But, this is still shown as a change? (I think because there is a new line that is not in the master branch?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added detail::
and needed to wrap the line, as it exceeded our line limit :)
* [MISC] deprecate seqan3::cigar_op and add changelog * Apply suggestions from code review
Resolves seqan/product_backlog#294.