-
-
Notifications
You must be signed in to change notification settings - Fork 17
Remove RTL markers from verse token data #378
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #378 +/- ##
=======================================
Coverage 72.61% 72.61%
=======================================
Files 423 423
Lines 35992 35995 +3
Branches 4965 4965
=======================================
+ Hits 26134 26137 +3
Misses 8760 8760
Partials 1098 1098 ☔ 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 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93).
src/SIL.Machine/Corpora/UsfmTokenizer.cs line 178 at r1 (raw file):
null, null, SanitizeVerseData(GetNextWord(usfm, ref index, preserveWhitespace))
I don't know if this is the right place to sanitize the verse data. It might be better to do this in UsfmParser when we create the verse ref.
src/SIL.Machine/Corpora/UsfmTokenizer.cs line 568 at r1 (raw file):
private static string SanitizeVerseData(string verseData) { return verseData.Replace("", "");
I assume that there is an RTL mark character in the string. It would be helpful to specify the character as a Unicode character literal, i.e. \u200f.
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 made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93).
src/SIL.Machine/Corpora/UsfmTokenizer.cs line 178 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I don't know if this is the right place to sanitize the verse data. It might be better to do this in
UsfmParserwhen we create the verse ref.
Generally, the tokenizer should try to preserve the original USFM as much as possible.
This was the cleanest place to make the edit, but I know you don't like to edit these classes if you don't have to. I'm open to alternatives but the token data itself needs to be sanitized in order for this to work when updating usfm. Also, added tests.
Fixes sillsdev/machine.py#250. This will need to be ported.
This change is