-
-
Notifications
You must be signed in to change notification settings - Fork 3
Port https://github.com/sillsdev/machine/pull/353 #245
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
|
Fixes #246. I started this a while ago and decided that if I didn't finish it now, it might get totally lost. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
==========================================
- Coverage 90.97% 90.61% -0.36%
==========================================
Files 338 347 +9
Lines 21804 21981 +177
==========================================
+ Hits 19837 19919 +82
- Misses 1967 2062 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
@ddaspit reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Enkidu93)
machine/corpora/usfm_versification_error_detector.py line 114 at r1 (raw file):
class UsfmVersificationErrorDetector(UsfmParserHandler):
You are missing types in this class.
tests/testutils/memory_paratext_project_versification_error_detector.py line 4 at r1 (raw file):
from machine.corpora import ParatextProjectSettings from machine.corpora.paratext_project_versification_error_detector import ParatextProjectVersificationErrorDetector
You should import from machine.corpora.
machine/corpora/zip_paratext_project_file_handler.py line 39 at r1 (raw file):
stylesheet_temp_fd, stylesheet_temp_path = mkstemp() with ( self.open(file_name) as source, # type: ignore
I would prefer that you cast rather than ignore the error. It makes the intent more explicit.
tests/testutils/memory_paratext_project_file_handler.py line 4 at r1 (raw file):
from typing import BinaryIO, Dict, Optional from machine.corpora.paratext_project_file_handler import ParatextProjectFileHandler
You should import from machine.corpora and machine.scripture.
tests/corpora/test_usfm_verisifcation_error_detector.py line 9 at r1 (raw file):
) from machine.corpora.paratext_project_settings import ParatextProjectSettings
You should import from machine.corpora and machine.scripture.
tests/corpora/test_usfm_manual.py line 22 at r1 (raw file):
UpdateUsfmTextBehavior, ) from machine.corpora.zip_paratext_project_versification_detector import ZipParatextProjectVersificationErrorDetector
You should import from machine.corpora.
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.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ddaspit)
machine/corpora/usfm_versification_error_detector.py line 114 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You are missing types in this class.
Do you mean type hints? Done - if I'm understanding correctly.
machine/corpora/zip_paratext_project_file_handler.py line 39 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I would prefer that you cast rather than ignore the error. It makes the intent more explicit.
OK, done.
tests/corpora/test_usfm_manual.py line 22 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You should import from
machine.corpora.
Done. I wonder if there's a way to fix the auto-importing. I guess it may have been that I hadn't added these classes to the __init__.py 🤔.
tests/corpora/test_usfm_verisifcation_error_detector.py line 9 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You should import from
machine.corporaandmachine.scripture.
Done.
tests/testutils/memory_paratext_project_file_handler.py line 4 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You should import from
machine.corporaandmachine.scripture.
Done.
tests/testutils/memory_paratext_project_versification_error_detector.py line 4 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You should import from
machine.corpora.
Done.
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.
@ddaspit reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)
machine/corpora/usfm_versification_error_detector.py line 114 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
Do you mean type hints? Done - if I'm understanding correctly.
Yes, that is what I meant. You forgot type hints for the method parameters.
tests/corpora/test_usfm_manual.py line 22 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
Done. I wonder if there's a way to fix the auto-importing. I guess it may have been that I hadn't added these classes to the
__init__.py🤔.
It is annoying. I like to interact with the classes in unit tests the same way that a calling application would, so that we can ensure that everything is exported properly.
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.
Reviewable status: 27 of 28 files reviewed, 1 unresolved discussion (waiting on @ddaspit)
machine/corpora/usfm_versification_error_detector.py line 114 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Yes, that is what I meant. You forgot type hints for the method parameters.
Oh, I see - sorry, yep! Surprises me that the type hints weren't carried over when VS Code suggested the method signatures from the base class 🤔. Maybe there's a setting for that too haha! Done.
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.
@ddaspit reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
machine/corpora/usfm_versification_error_detector.py line 114 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
Oh, I see - sorry, yep! Surprises me that the type hints weren't carried over when VS Code suggested the method signatures from the base class 🤔. Maybe there's a setting for that too haha! Done.
I noticed that it leaves out type hints.
Fix unused imports Fix import sorting Address reviewr comments Add parameter types Use isort
899b247 to
cb7a758
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.
@ddaspit reviewed 4 of 4 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
This change is