-
Notifications
You must be signed in to change notification settings - Fork 81
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] Switch from uppercase seqan3::field names to lower case. #1421
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1421 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 232 232
Lines 8825 8825
=======================================
Hits 8613 8613
Misses 212 212
Continue to review full report at Codecov.
|
190ee08
to
d9d95e3
Compare
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.
Looks perfect, I have cross-checked all the field names.
USER_DEFINED_6 SEQAN3_DEPRECATED_310, //!< Please use the field name in lower case. | ||
USER_DEFINED_7 SEQAN3_DEPRECATED_310, //!< Please use the field name in lower case. | ||
USER_DEFINED_8 SEQAN3_DEPRECATED_310, //!< Please use the field name in lower case. | ||
USER_DEFINED_9 SEQAN3_DEPRECATED_310, //!< Please use the field name in lower case. |
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.
Do we really need this?
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.
Well since this is actually already used rather often, I think it is important to provide a clear message to the user. Additionally this does not hurt much, does it?
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.
@rrahn Do you agree?
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.
yes it is fine.
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.
Only one open question.
Resolves #1422