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

Refactor functions only required by config/utils under config #432

Merged
merged 2 commits into from
May 22, 2024

Conversation

joanise
Copy link
Member

@joanise joanise commented May 21, 2024

PR Goal?

Triggered by the fact that everyvoice/utils.py was importing Pydantic (agreed, I'm probably unreasonable about that), this PR tidies some function definitions: everyvoice/config/utils.py exists to provide validators, and their definitions depend on five functions that are used nowhere else, so I've moved them all away to everyvoice/config/validation_helpers.py.

As a welcome side effect, everyvoice -h no longer imports Pydantic, but this refactoring has value in and of itself in my opinion.

Secondary change: since CI now complains if everyvoice -h imports Pydantic, I added an equivalent unit test so we can see it fail locally, earlier, as soon as we cause the issue. I agree banning Pydantic from everyvoice -h might be a bit extreme, though, and am willing to rediscuss.

Fixes?

Eric's frustration, plus a bit of technical debt. And passes CI.

Feedback sought?

normal review

Priority?

Tests added?

this code was already well covered

How to test?

Confidence?

high

Version change?

no

Related PRs?

none

Copy link
Contributor

CLI load time: 0:00.25
Pull Request HEAD: 237b983f90f82f18c56d261215759cd2b751558b
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 91.52542% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 73.94%. Comparing base (e8a6da2) to head (237b983).

Files Patch % Lines
everyvoice/config/validation_helpers.py 91.07% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           dev.ap/vocoder-match     #432      +/-   ##
========================================================
+ Coverage                 73.79%   73.94%   +0.15%     
========================================================
  Files                        42       43       +1     
  Lines                      2583     2618      +35     
  Branches                    396      403       +7     
========================================================
+ Hits                       1906     1936      +30     
- Misses                      599      602       +3     
- Partials                     78       80       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@roedoejet roedoejet left a comment

Choose a reason for hiding this comment

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

I like this @joanise - looks good to me.

@roedoejet roedoejet merged commit fe43421 into dev.ap/vocoder-match May 22, 2024
6 checks passed
@roedoejet roedoejet deleted the dev.ej/pydantic-muck branch May 22, 2024 16:52
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.

2 participants