-
Notifications
You must be signed in to change notification settings - Fork 1
Code improvements while working on EV PR 758 #130
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
Changed Files
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
=======================================
Coverage 49.64% 49.64%
=======================================
Files 27 27
Lines 1982 1984 +2
=======================================
+ Hits 984 985 +1
- Misses 998 999 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fbf74c2 to
83f349e
Compare
roedoejet
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.
just a clarifying question, looks good to me though
| default_language: str | None = None, | ||
| default_speaker: str | None = None, | ||
| output_type: list[SynthesizeOutputFormats] = [], | ||
| output_type: Sequence[SynthesizeOutputFormats] = [], |
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.
What's the reason for this? don't we just mean it to be a list?
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.
Sometimes we pass tuples in, and that's a type mismatch -- we do so in unit tests for sure, maybe elsewhere I forget. I figured it made more sense to make the type annotation more flexible than force the use of actual lists everywhere, given any sequence will behave correctly here.
See EveryVoiceTTS/EveryVoice#759