-
-
Notifications
You must be signed in to change notification settings - Fork 2
Add disclaimer by passing remark to Machine #681
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
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 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)
src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 965 at r1 (raw file):
embedBehavior ?? PretranslationUsfmMarkerBehavior.Preserve, styleMarkerBehavior ?? PretranslationUsfmMarkerBehavior.Strip, [string.Format(CultureInfo.InvariantCulture, DisclaimerRemarkText, textId)], //What's the least misleading, easy-to-implement date to give here? Or should we hold off on a date and add it in a future PR? What about mentioning that it's been drafted using a fine-tuned NLLB?
I would specify the remark in the pretranslation service. I don't think there is a need to pass it from the controller. We should use the "u" format specifier to format the date/time.
OK, I assume we should use the latest build's |
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 4 of 4 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 965 at r1 (raw file):
Previously, Enkidu93 (Eli C. Lowry) wrote…
OK, I assume we should use the latest build's
DateFinished. I was mainly asking whether I should use theDateFinishedor the date when the USFM itself was generated using the existing pretranslations. The second date could be helpful if settings were changed in XForge that affected the USFM output or general Serval changes were made. Then the user would know that they should grab the USFM again in order for those changes to take affect. In general, I think using theDateFinishedof the build would be more what the user would expect. What do you think?
Oh, I see. Yes, I would use the DateFinished from the build.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #681 +/- ##
==========================================
- Coverage 66.08% 66.06% -0.02%
==========================================
Files 342 342
Lines 18836 18854 +18
Branches 2455 2456 +1
==========================================
+ Hits 12447 12456 +9
- Misses 5492 5499 +7
- Partials 897 899 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Still fixing tests |
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 3 of 4 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)
Fixes #542
Continuing work here
Machine-side PR: sillsdev/machine#301
This change is