Add Paratext metadata to project data files#931
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #931 +/- ##
=======================================
Coverage 64.63% 64.64%
=======================================
Files 379 381 +2
Lines 21646 21694 +48
Branches 2763 2765 +2
=======================================
+ Hits 13991 14024 +33
- Misses 6631 6645 +14
- Partials 1024 1025 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
c3d9619 to
84f1ebe
Compare
Thanks for the review 😁! Just making sure you saw my question above: While we're at it, is there any other Paratext information that would be helpful to stow here? |
ddaspit
left a comment
There was a problem hiding this comment.
I missed your questions in the description. I responded to them in the comments.
@ddaspit made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93).
src/Serval/src/Serval.DataFiles/Models/DataFile.cs line 11 at r1 (raw file):
public string Filename { get; init; } = ""; public required FileFormat Format { get; init; } public ParatextMetadata? ParatextMetadata { get; init; }
I would like this property to be extensible for different formats, but I also like the type-safety of a statically typed class. Since we only support metadata for the Paratext format, we can just use the ParatextMetadata as the type but rename the field to FileMetadata. In the future, if we want to support different metadata for other formats, we can create a custom serializer that deserializes to the correct metadata class based on the value of the Format field.
src/Serval/src/Serval.DataFiles/Models/ParatextMetadata.cs line 3 at r1 (raw file):
namespace Serval.DataFiles.Models; public record ParatextMetadata
I think it would make sense to include the language code, translation type, and versification. I think that would be the most useful metadata. What do you think?
|
@Enkidu93 You may want to consider including the FullName and perhaps the Visibility? Some projects have a Visibility of Confidential, and in it can be helpful to know that we have to deal quite sensitively with that project (on SF we store Visibility in the sf_project Mongo document for that reason). |
Enkidu93
left a comment
There was a problem hiding this comment.
Thanks, Peter! I went ahead and added those. The visibility will require a new Machine version. See sillsdev/machine#412.
I also added a test.
@Enkidu93 made 3 comments.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on ddaspit).
src/Serval/src/Serval.DataFiles/Models/DataFile.cs line 11 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I would like this property to be extensible for different formats, but I also like the type-safety of a statically typed class. Since we only support metadata for the
Paratextformat, we can just use theParatextMetadataas the type but rename the field toFileMetadata. In the future, if we want to support different metadata for other formats, we can create a custom serializer that deserializes to the correct metadata class based on the value of theFormatfield.
Done. Let me know if this is what you wanted.
src/Serval/src/Serval.DataFiles/Models/ParatextMetadata.cs line 3 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
I think it would make sense to include the language code, translation type, and versification. I think that would be the most useful metadata. What do you think?
I went ahead added the fields you suggested as well as those Peter mentioned.
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 8 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
919fb02 to
643ad72
Compare
|
Waiting for #934 to go through first |
6fd410b to
0dea5aa
Compare
|
@ddaspit If you could take a look to see if I properly merged my changes given your data files vertical slice refactoring, that would be great! |
Reviewer comments Add additional fields to metadata; add test
5235f47 to
a7964c7
Compare
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 11 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
Fixes #902.
Is there other data we'd like to put in here? Language code? Translation type (e.g. BackTranslation)?
I wasn't sure @ddaspit if you wanted a generic metadata field. I can do that if you prefer.
I had to avoid a naming collision with the type
Guidthus the slightly verbose property names.This change is