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

Detect empty mappings and fail early. #323

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Conversation

joanise
Copy link
Collaborator

@joanise joanise commented Feb 13, 2024

Empty mappings cause the g2p database to get corrupted, so block them.
Other changes were things I noticed while fixing this, including some coverage improvements.

Fixes: #322

@joanise joanise requested a review from dhdaines February 13, 2024 20:06
Copy link
Contributor

github-actions bot commented Feb 13, 2024

CLI load time: 0:00.05
PR head 95bf4be192cbeb24d9c78da4318ff050f46dedcf
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (26c4d4a) 92.54% compared to head (7f0739e) 92.95%.

❗ Current head 7f0739e differs from pull request most recent head 95bf4be. Consider uploading reports for the commit 95bf4be to get more accurate results

Files Patch % Lines
g2p/app.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
+ Coverage   92.54%   92.95%   +0.40%     
==========================================
  Files          18       18              
  Lines        2334     2341       +7     
  Branches      517      519       +2     
==========================================
+ Hits         2160     2176      +16     
+ Misses         99       94       -5     
+ Partials       75       71       -4     

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

Copy link
Collaborator

@dhdaines dhdaines left a comment

Choose a reason for hiding this comment

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

just a couple of little changes, otherwise good!

g2p/mappings/__init__.py Show resolved Hide resolved
g2p/mappings/utils.py Show resolved Hide resolved
@joanise
Copy link
Collaborator Author

joanise commented Feb 13, 2024

@dhdaines Unit test added, ready to re-review.

@joanise joanise requested a review from dhdaines February 13, 2024 22:29
Letting an empty mapping get loaded will corrupt langs.json.gz, so
immediately alert the user they cannot do that!

Also, don't override the rules_path if it's already initialized.

Fixes: 322
Also, no need to sleep after starting the API since it has the whole
run_tests.py time to launch itself.

Also, network.json.gz was missed in the files whose status we checked
after g2p update.
Pydantic validation errors were previously causing stack trace dumps
@joanise joanise force-pushed the dev.ej/322.report-empty-mapping branch from 7f0739e to 95bf4be Compare February 13, 2024 22:34
@joanise joanise merged commit 95bf4be into main Feb 13, 2024
4 checks passed
@joanise joanise deleted the dev.ej/322.report-empty-mapping branch February 13, 2024 22:35
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.

Creating an empty mapping corrupts the langs.json.gz file
2 participants