-
-
Notifications
You must be signed in to change notification settings - Fork 3
Fix type for Russian versifications #217
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
Conversation
1dfdd34 to
b45eac3
Compare
|
This is a fix for an issue in silnlp. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
=======================================
Coverage 91.11% 91.11%
=======================================
Files 333 333
Lines 21697 21706 +9
=======================================
+ Hits 19769 19778 +9
Misses 1928 1928 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Enkidu93
left a comment
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ddaspit)
benjaminking
left a comment
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ddaspit)
b45eac3 to
e21994f
Compare
| def test_builtin_versification_type() -> None: | ||
| assert ENGLISH_VERSIFICATION.type == VersificationType.ENGLISH | ||
| assert ORIGINAL_VERSIFICATION.type == VersificationType.ORIGINAL | ||
| assert RUSSIAN_ORTHODOX_VERSIFICATION.type == VersificationType.RUSSIAN_ORTHODOX |
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.
@ddaspit Since there are only 6 types, why not just have an assertion for each of them?
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.
Done
e21994f to
f3e68ed
Compare
f3e68ed to
ed5e55c
Compare
ddaspit
left a comment
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.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @benjaminking, @Enkidu93, and @jcuenod)
| def test_builtin_versification_type() -> None: | ||
| assert ENGLISH_VERSIFICATION.type == VersificationType.ENGLISH | ||
| assert ORIGINAL_VERSIFICATION.type == VersificationType.ORIGINAL | ||
| assert RUSSIAN_ORTHODOX_VERSIFICATION.type == VersificationType.RUSSIAN_ORTHODOX |
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.
Done
This change is