#370 - control pretranslating existing text

[johnml1135]

I didn't need to add another parameter for pretranslating stuff that is not being trained on. It seemed obvious (to me) that if a person is actively choosing not to train a specific verse, and asks for it pretranslated, we should just do what they want (credit to Damien for the idea).

This change is 

[ddaspit]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @johnml1135)

---

src/Serval.Translation/Contracts/PretranslationUSFMTextOrigin.cs line 3 at r1 (raw file):

namespace Serval.Translation.Contracts;

public enum PretranslationUSFMTextOrigin

For acronyms that are longer than 2 characters, you should only capitalize the first letter.

---

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 660 at r1 (raw file):

        [NotNull] string corpusId,
        [NotNull] string textId,
        [FromBody] PretranslationUSFMTextOrigin? textOrigin,

It is unexpected and often considered incorrect to pass a body when using the GET method. This can be specified using a query string, i.e. [FromQuery]. Since it will be part of the URL, the field name needs to be kebab case. You can easily specify the field name in the attribute, i.e. [FromQuery(Name = "text-origin")].

---

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 676 at r1 (raw file):

            corpusId,
            textId,
            textOrigin ??= PretranslationUSFMTextOrigin.PreferExisting,

You only need the normal null-coalescing operator (??).

---

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 774 at r1 (raw file):

    /// </summary>
    /// <remarks>
    /// Specify the corpora and textIds to train on. If no train_on field is provided, all corpora will be used.

train_on should be trainOn. We should also put it in quotes so that it is obvious that we are referring to a property.

---

src/Serval.Translation/Services/PretranslationService.cs line 37 at r1 (raw file):

        string corpusId,
        string textId,
        PretranslationUSFMTextOrigin textOrigin = PretranslationUSFMTextOrigin.PreferExisting,

This doesn't need to have a default value, especially since there isn't one specified in the interface.

---

src/Serval.Translation/Services/PretranslationService.cs line 129 at r1 (raw file):

                        sourceSettings,
                        usfm,
                        [], // don't pass the pretranslations, we only want the existing text.

When we pass a literal value, I usually explicitly specify the parameter name, since it isn't obvious from the variable name.

[johnml1135]

src/Serval.Translation/Contracts/PretranslationUSFMTextOrigin.cs line 3 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

For acronyms that are longer than 2 characters, you should only capitalize the first letter.

done

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 660 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

It is unexpected and often considered incorrect to pass a body when using the GET method. This can be specified using a query string, i.e. [FromQuery]. Since it will be part of the URL, the field name needs to be kebab case. You can easily specify the field name in the attribute, i.e. [FromQuery(Name = "text-origin")].

Done.

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 676 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You only need the normal null-coalescing operator (??).

Done.

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 774 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

train_on should be trainOn. We should also put it in quotes so that it is obvious that we are referring to a property.

Done.

[johnml1135]

src/Serval.Translation/Services/PretranslationService.cs line 37 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This doesn't need to have a default value, especially since there isn't one specified in the interface.

Done.

[johnml1135]

src/Serval.Translation/Services/PretranslationService.cs line 129 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

When we pass a literal value, I usually explicitly specify the parameter name, since it isn't obvious from the variable name.

Done.

[johnml1135]

Write more tests, and update machine version.

[ddaspit]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

---

tests/Serval.Translation.Tests/Services/PretranslationServiceTests.cs line 15 at r2 (raw file):

            "corpus1",
            "MAT",
            textOrigin: Contracts.PretranslationUsfmTextOrigin.PreferPretranslated

If you add global using Serval.Translation.Contracts; to "Usings.cs", then you can get rid of the namespace.

[johnml1135]

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @ddaspit)

---

tests/Serval.Translation.Tests/Services/PretranslationServiceTests.cs line 15 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

If you add global using Serval.Translation.Contracts; to "Usings.cs", then you can get rid of the namespace.

Done.

[ddaspit]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

[ddaspit]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

---

src/Serval.Translation/Services/PretranslationService.cs line 126 at r3 (raw file):

                    );
                case PretranslationUsfmTextOrigin.OnlyExisting:
                    return UpdateUsfm(

In this case, we are just returning the USFM with all text stripped out. Is that correct?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.55224% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 70.81%. Comparing base (624d4b4) to head (e4d83d0).

Files	Patch %	Lines
src/Serval.Client/Client.g.cs	33.33%	3 Missing and 1 partial ⚠️
...rval.Translation/Services/PretranslationService.cs	95.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
+ Coverage   70.70%   70.81%   +0.11%     
==========================================
  Files         151      151              
  Lines        7114     7172      +58     
  Branches     1118     1121       +3     
==========================================
+ Hits         5030     5079      +49     
- Misses       1633     1638       +5     
- Partials      451      455       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[johnml1135]

src/Serval.Translation/Services/PretranslationService.cs line 126 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

In this case, we are just returning the USFM with all text stripped out. Is that correct?

Yes.

[johnml1135]

Reviewed 1 of 6 files at r2, 6 of 6 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)