Update machine.py to reflect usfm changes in machine #103
Conversation
There was a problem hiding this comment.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mshannon-sil)
machine/corpora/paratext_backup_terms_corpus.py line 15 at r1 (raw file):
class ParatextBackupTermsCorpus(DictionaryTextCorpus): def __init__(self, filename: str, term_categories: List[str]) -> None: self._predefined_terms_list_types = ["Major", "All", "SilNt", "Pt6"]
You can move this to a constant (_PREDEFINED_TERMS_LIST_TYPES) at the top of the file.
machine/corpora/paratext_backup_terms_corpus.py line 61 at r1 (raw file):
self._add_text(text) def _get_renderings(self, rendering: str) -> List[str]:
You can refactor these methods to functions at the top level of the file. Generally, I port any private static methods to module-level "private" functions.
machine/corpora/paratext_project_settings.py line 7 at r1 (raw file):
class ParatextProjectSettings(ABC):
This can be a dataclass.
machine/corpora/paratext_project_settings_parser_base.py line 15 at r1 (raw file):
@abstractmethod def __enter__(self) -> "ParatextProjectSettingsParserBase": ...
I don't think this class needs to be a context manager.
machine/corpora/zip_paratext_project_settings_parser.py line 20 at r1 (raw file):
return file_name in self._archive.namelist() def find(self, extension: str) -> Union[str, None]:
Optional[str] is better than Union[str, None].
machine/corpora/zip_paratext_project_settings_parser_base.py line 15 at r1 (raw file):
stylesheet_temp_file.write(source.read()) stylesheet_path = stylesheet_temp_file.name custom_stylesheet_path = None
You forgot to specify the type here.
tests/corpora/test_usfm_verse_text_updater.py line 5 at r1 (raw file):
from testutils.corpora_test_helpers import USFM_TEST_PROJECT_PATH from machine.corpora.usfm_parser import parse_usfm
You should import from the package, i.e. from machine.corpora import parse_usfm.
tests/corpora/test_usfm_verse_text_updater.py line 13 at r1 (raw file):
rows = [ ( [VerseRef.from_string("MAT 1:1", Versification.get_builtin(VersificationType.ENGLISH))],
You can import the versification as a constant from machine.scripture, i.e. from machine.scripture import ENGLISH_VERSIFICATION.
There was a problem hiding this comment.
Reviewable status: 13 of 21 files reviewed, 8 unresolved discussions (waiting on @ddaspit and @mshannon-sil)
machine/corpora/paratext_backup_terms_corpus.py line 15 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You can move this to a constant (
_PREDEFINED_TERMS_LIST_TYPES) at the top of the file.
Done.
machine/corpora/paratext_backup_terms_corpus.py line 61 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You can refactor these methods to functions at the top level of the file. Generally, I port any private static methods to module-level "private" functions.
Done.
machine/corpora/paratext_project_settings.py line 7 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
This can be a dataclass.
Done.
machine/corpora/paratext_project_settings_parser_base.py line 15 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I don't think this class needs to be a context manager.
Done.
machine/corpora/zip_paratext_project_settings_parser.py line 20 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Optional[str]is better thanUnion[str, None].
Done.
machine/corpora/zip_paratext_project_settings_parser_base.py line 15 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You forgot to specify the type here.
Done.
tests/corpora/test_usfm_verse_text_updater.py line 5 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You should import from the package, i.e.
from machine.corpora import parse_usfm.
Done.
tests/corpora/test_usfm_verse_text_updater.py line 13 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
You can import the versification as a constant from
machine.scripture, i.e.from machine.scripture import ENGLISH_VERSIFICATION.
Done. Also, there's a handful of other places in the codebase that use Versification.get_builtin(VersificationType.ENGLISH) rather than ENGLISH_VERSIFICATION. Would you like me to change those too so that it's consistent?
|
This PR is porting over the changes from sillsdev/machine#163 as well now. |
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mshannon-sil)
machine/corpora/paratext_project_settings.py line 7 at r1 (raw file):
Previously, mshannon-sil wrote…
Done.
I don't think it needs to be an abstract base class.
tests/corpora/test_usfm_verse_text_updater.py line 13 at r1 (raw file):
Previously, mshannon-sil wrote…
Done. Also, there's a handful of other places in the codebase that use
Versification.get_builtin(VersificationType.ENGLISH)rather thanENGLISH_VERSIFICATION. Would you like me to change those too so that it's consistent?
I believe that there are a couple of places that need to use ``Versification.get_builtin()`, but other than that everywhere else should use the constants.
There was a problem hiding this comment.
Reviewable status: 19 of 25 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @mshannon-sil)
machine/corpora/paratext_project_settings.py line 7 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I don't think it needs to be an abstract base class.
Done.
tests/corpora/test_usfm_verse_text_updater.py line 13 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I believe that there are a couple of places that need to use ``Versification.get_builtin()`, but other than that everywhere else should use the constants.
Okay, I changed most of the places that I saw using Versification.get_builtin(VersificationType.ENGLISH) to use ENGLISH_VERSIFICATION instead, except in verse_ref.py to avoid circular imports. All the test cases are still passing. Let me know if there's any of those places that I need to change back to the original method.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @mshannon-sil)
mshannon-sil
changed the title
Based on the changes in Machine from sillsdev/machine#160, I've updated machine.py's handling of USFM accordingly.
Main changes:
machine/corporawere either created or modified to align with new files/changes in MachineVersification.load()inmachine/scripture/verse_ref.pywas updated to accept a stream as input, like it can in Machinetest_utf_16_encoding_stream()intests/scripture/test_versification.pyto test code I added which handles Unicode errors for loading versification from a stream.This change is