Skip to content
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 ability to use localizations for key terms #454

Merged
merged 17 commits into from
Aug 22, 2024
Merged

Add ability to use localizations for key terms #454

merged 17 commits into from
Aug 22, 2024

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Aug 15, 2024

Fixes #240

Parallel PR: sillsdev/machine#233


This change is Reviewable

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)


src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 171 at r1 (raw file):

                (string sourceLanguageCode, string targetLanguageCode) = languageCodes;
                ITextCorpus? sourceTermCorpus = _corpusService
                    .CreateTermCorpora(corpus.SourceFiles, sourceLanguageCode)

I thought we decided to use the language code from the PT project settings.

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a 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 5 files reviewed, 1 unresolved discussion (waiting on @ddaspit and @johnml1135)


src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 171 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I thought we decided to use the language code from the PT project settings.

Sorry, yes, done. I had put that together in my mind with the later issue of adding the Chinese localizations, but better just to do it now anyways.

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddaspit, assuming you're OK with the changes in Machine, should I use the Serval IZipContainer here instead of ZipArchive?

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

@johnml1135
Copy link
Collaborator

src/Machine/src/Serval.Machine.Shared/Services/CorpusService.cs line 48 at r3 (raw file):

                    using (var archive = ZipFile.OpenRead(file.Location))
                    {
                        yield return new ZipParatextProjectTermsCorpus(archive, ["PN"]);

This will need a new version of Machine released (and included) before it can be approved.

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ddaspit and @Enkidu93)

@johnml1135
Copy link
Collaborator

src/Machine/src/Serval.Machine.Shared/Services/CorpusService.cs line 48 at r3 (raw file):

Previously, johnml1135 (John Lambert) wrote…

This will need a new version of Machine released (and included) before it can be approved.

This has been resolved.

Copy link
Collaborator

@johnml1135 johnml1135 left a 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 r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.68%. Comparing base (8732b40) to head (e808ca0).

Files Patch % Lines
...rval.Machine.Shared/Services/PreprocessBuildJob.cs 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #454   +/-   ##
=======================================
  Coverage   56.68%   56.68%           
=======================================
  Files         275      275           
  Lines       14115    14115           
  Branches     1895     1895           
=======================================
  Hits         8001     8001           
  Misses       5529     5529           
  Partials      585      585           

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

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r2, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@Enkidu93 Enkidu93 merged commit 661eb1a into main Aug 22, 2024
3 checks passed
@Enkidu93 Enkidu93 deleted the key_terms branch August 22, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SF integration: use of Key Biblical Terms as training data
4 participants