-
-
Notifications
You must be signed in to change notification settings - Fork 3
Pass metadata through update block #202
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
isaac091
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: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @ddaspit)
machine/corpora/update_usfm_parser_handler.py line 314 at r1 (raw file):
# grab the text - both source and row will be incremented in due time... row_texts.append(text) row_metadata = metadata
No issues that I can think of
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.
Yes, I think it makes sense to define alignment_info as a constant.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Enkidu93)
machine/corpora/place_markers_usfm_update_block_handler.py line 12 at r1 (raw file):
class PlaceMarkersAlignmentInfo(TypedDict):
I think this is only used in unit tests. We could cast alignment_info to this class when we use it. See my comment below. If we don't use it here, we should just move it to the unit test.
machine/corpora/place_markers_usfm_update_block_handler.py line 28 at r1 (raw file):
len(elements) == 0 or "alignment_info" not in block.metadata or not isinstance(block.metadata["alignment_info"], dict)
It is correct to thoroughly validate the metadata before using it. We could probably get away with doing less. You could just check that alignment_info exists, and then cast it to PlaceMarkersAlignmentInfo. We just assume that the data structure is correct if alignment_info exists. If it isn't correct, it will probablly cause a crash, which is fine, because it would indicate a coding error.
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.
OK, done.
Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @ddaspit and @isaac091)
machine/corpora/place_markers_usfm_update_block_handler.py line 12 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I think this is only used in unit tests. We could cast
alignment_infoto this class when we use it. See my comment below. If we don't use it here, we should just move it to the unit test.
Makes sense
machine/corpora/place_markers_usfm_update_block_handler.py line 28 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
It is correct to thoroughly validate the metadata before using it. We could probably get away with doing less. You could just check that
alignment_infoexists, and then cast it toPlaceMarkersAlignmentInfo. We just assume that the data structure is correct ifalignment_infoexists. If it isn't correct, it will probablly cause a crash, which is fine, because it would indicate a coding error.
OK, done.
machine/corpora/update_usfm_parser_handler.py line 314 at r1 (raw file):
Previously, isaac091 (Isaac Schifferer) wrote…
No issues that I can think of
Cool
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #202 +/- ##
==========================================
- Coverage 88.91% 88.89% -0.03%
==========================================
Files 282 282
Lines 17056 17050 -6
==========================================
- Hits 15166 15156 -10
- Misses 1890 1894 +4 ☔ 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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @isaac091)
Fixes #193
This change is