-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add UsfmVerseTextUpdater class #160
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 66.04% 66.59% +0.55%
==========================================
Files 428 433 +5
Lines 33529 33738 +209
Branches 4511 4525 +14
==========================================
+ Hits 22144 22469 +325
+ Misses 10301 10200 -101
+ Partials 1084 1069 -15 ☔ View full report in Codecov by Sentry. |
Wouldn't you put the usings into their own file? |
Previously, johnml1135 (John Lambert) wrote…
Ah, older version of .net. |
So, we default to Matthew? Shouldn't we default to crashing? I don't think I fully understand this logic and intention. |
Part of the general question of if the data is not meeting expectations. I don't know the history of this code (I am assuming it is copied from somewhere else with minor edits). If that is the case, are there any substantive logic changes? |
Previously, johnml1135 (John Lambert) wrote…
How to handle missing data, exceptions, etc. |
There are so many ways to fail, including not getting 3 sections. Are we ok with just throwing an exception and failing the process? |
For my information, what is the difference between ParatextTextCorpus and ParatextBackupTextCorpus? |
I'm assuming this is just a naming update. Descriptive names are better than comments? |
Ah, the backup is using the zipped one... |
Previously, johnml1135 (John Lambert) wrote…
Ah - one grabs from a zip file, the other from files on the computer. |
A really simple explanation of what this is doing would be great. This class is not used in machine - likely only in Serval (which hasn't been pushed yet). It appears to be used to update an existing USFM file with pretranslations - but the logic appears split between a few locations. |
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 13 files reviewed, 8 unresolved discussions (waiting on @johnml1135)
src/SIL.Machine/Corpora/FileParatextProjectSettingsParser.cs
line 5 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
Ah, older version of .net.
Yes, that is correct.
src/SIL.Machine/Corpora/ParatextBackupTermsCorpus.cs
line 31 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
Ah, the backup is using the zipped one...
Yes, that is correct.
src/SIL.Machine/Corpora/ParatextProjectSettingsParserBase.cs
line 67 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
So, we default to Matthew? Shouldn't we default to crashing? I don't think I fully understand this logic and intention.
These are the defaults in Paratext for these settings. In this case, it indicates the file name pattern for USFM files in the project.
src/SIL.Machine/Corpora/ParatextProjectSettingsParserBase.cs
line 69 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
How to handle missing data, exceptions, etc.
Yes, this code was essentially extracted from another class and made to be reusable.
src/SIL.Machine/Corpora/ParatextProjectSettingsParserBase.cs
line 86 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
There are so many ways to fail, including not getting 3 sections. Are we ok with just throwing an exception and failing the process?
Throwing exceptions is the right approach. If anything throws an exception, then the settings file is corrupt or invalid and we don't know how to interpret it. We could always add in more descriptive exceptions later.
src/SIL.Machine/Corpora/ParatextTextCorpus.cs
line 9 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
Ah - one grabs from a zip file, the other from files on the computer.
Yes, that is correct.
src/SIL.Machine/Corpora/UsfmParser.cs
line 47 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
I'm assuming this is just a naming update. Descriptive names are better than comments?
This field was moved to a public property in the parser state, so that it is available while parsing.
src/SIL.Machine/Corpora/UsfmVerseTextUpdater.cs
line 117 at r1 (raw file):
Previously, johnml1135 (John Lambert) wrote…
A really simple explanation of what this is doing would be great. This class is not used in machine - likely only in Serval (which hasn't been pushed yet). It appears to be used to update an existing USFM file with pretranslations - but the logic appears split between a few locations.
Done.
There is a bit of a lack of coverage for the TextUpdater, specifically the Sidebar, Milestone, Ref, OptBreak , Unmatched. Also, there is some missing coverage in the other files (as seen above in the report). |
Previously, ddaspit (Damien Daspit) wrote…
In aerospace, if code has been working for a long time, it's grandfathered in. That's fine - we can address issues if they come up. |
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 13 of 13 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)
I would like more testing for these cases... |
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.
I added more unit tests.
Dismissed @johnml1135 from a discussion.
Reviewable status: 11 of 18 files reviewed, all discussions resolved (waiting on @johnml1135)
I suspected that there could be an issue there :-) |
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 7 of 7 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)
This change is