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

[MISC] Alphabet: rename phred68legacy to phred68solexa #2522

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Apr 19, 2021

@Irallia Irallia self-assigned this Apr 19, 2021
@vercel
Copy link

vercel bot commented Apr 19, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/G14mU9rJ497WYDuSo5FM7mevMjRw
✅ Preview: https://seqan3-git-fork-irallia-misc-alphabetrenamephred-b01cbe.vercel.app

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #2522 (6f875d1) into master (50af89f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2522   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         269      269           
  Lines       10521    10521           
=======================================
  Hits        10334    10334           
  Misses        187      187           
Impacted Files Coverage Δ
include/seqan3/alphabet/quality/phred68solexa.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 50af89f...6f875d1. Read the comment docs.

@marehr
Copy link
Member

marehr commented Apr 19, 2021

Snippets failed

 [ 65%] Building CXX object CMakeFiles/phred68solexa_snippet.dir/alphabet/quality/phred68solexa.cpp.o
In file included from /home/runner/work/seqan3/seqan3/seqan3/test/snippet/alphabet/quality/phred68solexa.cpp:1:0:
/home/runner/work/seqan3/seqan3/seqan3/include/seqan3/alphabet/quality/phred68legacy.hpp:26:13: error: This header is deprecated and will be removed in SeqAn-3.1.0; Please #include <seqan3/alphabet/quality/phred68solexa.hpp> instead. [-Werror]
    "This header is deprecated and will be removed in SeqAn-3.1.0; Please #include <seqan3/alphabet/quality/phred68solexa.hpp> instead.")
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~        
cc1plus: all warnings being treated as errors
make[2]: *** [CMakeFiles/phred68solexa_snippet.dir/build.make:63: CMakeFiles/phred68solexa_snippet.dir/alphabet/quality/phred68solexa.cpp.o] Error 1
make[2]: Target 'CMakeFiles/phred68solexa_snippet.dir/build' not remade because of errors.
make[1]: *** [CMakeFiles/Makefile2:10534: CMakeFiles/phred68solexa_snippet.dir/all] Error 2

@marehr marehr requested review from a team and MitraDarja and removed request for a team April 19, 2021 13:10
@Irallia Irallia force-pushed the misc/alphabet/rename_phred68legacy_to_phred68solexa branch from 5849cb2 to 0d7da40 Compare April 19, 2021 14:01
@Irallia Irallia force-pushed the misc/alphabet/rename_phred68legacy_to_phred68solexa branch from 0d7da40 to ddcc169 Compare April 19, 2021 14:11
@MitraDarja MitraDarja requested review from a team and marehr and removed request for a team April 19, 2021 14:56
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -32,6 +32,8 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.

* Added `seqan3::phred94`, a quality type that represents the full Phred Score range (Sanger format) and is used for
PacBio Phred scores of HiFi reads ([\#2290](https://github.com/seqan/seqan3/pull/2290)).
* Deprecated `seqan3::phred68legacy`. Use `seqan3::phred68solexa` instead.
([\#...](https://github.com/seqan/seqan3/pull/2522)).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
([\#...](https://github.com/seqan/seqan3/pull/2522)).
([\#2522](https://github.com/seqan/seqan3/pull/2522)).

Copy link
Member

Choose a reason for hiding this comment

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

I would rather write renamed X to Y
Deprecation is usually more for stuff that does not exist anymore

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@marehr marehr requested review from eseiler and removed request for marehr April 19, 2021 15:48
Copy link
Member

@eseiler eseiler left a comment

Choose a reason for hiding this comment

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

💅 + the stuff I wrote as comment before

CHANGELOG.md Outdated
@@ -32,6 +32,8 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.

* Added `seqan3::phred94`, a quality type that represents the full Phred Score range (Sanger format) and is used for
PacBio Phred scores of HiFi reads ([\#2290](https://github.com/seqan/seqan3/pull/2290)).
* Deprecated `seqan3::phred68legacy`. Use `seqan3::phred68solexa` instead.
([\#...](https://github.com/seqan/seqan3/pull/2522)).
Copy link
Member

Choose a reason for hiding this comment

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

I would rather write renamed X to Y
Deprecation is usually more for stuff that does not exist anymore

marehr and others added 3 commits April 20, 2021 12:00
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia force-pushed the misc/alphabet/rename_phred68legacy_to_phred68solexa branch from eb9196b to cf52cf8 Compare April 20, 2021 10:00
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
eseiler
eseiler approved these changes Apr 20, 2021
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.

rename phred68{legacy,solexa}
4 participants