Repository navigation
Avoid false 'multiple versifications' warnings - #1051
Conversation
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman reviewed 6 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93).
src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 318 at r1 (raw file):
new Dictionary<string, object> { { "projectVersifications", projectVersifications } } ) );
Will this store the entire ScrVers objects in the diagnostics, which are written to Mongo? I think this should be:
diagnostics.Add(
BuildDiagnosticService.CreateDiagnostic(
"CONFIG-0002",
new Dictionary<string, object>
{
{
"projectVersifications",
projectVersifications.ToDictionary(v => v.Key, v => v.Value.Name)
},
}
)
);Code quote:
diagnostics.Add(
BuildDiagnosticService.CreateDiagnostic(
"CONFIG-0002",
new Dictionary<string, object> { { "projectVersifications", projectVersifications } }
)
);src/Machine/test/Serval.Machine.Translation.Tests/Services/PreprocessBuildJobTests.cs line 102 at r1 (raw file):
"pt-target1", "1111", ScrVers.Original,
This should be ScrVers.English (RunAsync_BuildWarnings fails)
Code quote:
ScrVers.Original,
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).
src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 318 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Will this store the entire
ScrVersobjects in the diagnostics, which are written to Mongo? I think this should be:diagnostics.Add( BuildDiagnosticService.CreateDiagnostic( "CONFIG-0002", new Dictionary<string, object> { { "projectVersifications", projectVersifications.ToDictionary(v => v.Key, v => v.Value.Name) }, } ) );
Done.
src/Machine/test/Serval.Machine.Translation.Tests/Services/PreprocessBuildJobTests.cs line 102 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
This should be
ScrVers.English(RunAsync_BuildWarningsfails)
Yes, sorry. Done.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1051 +/- ##
==========================================
+ Coverage 72.82% 72.83% +0.01%
==========================================
Files 391 391
Lines 21735 21745 +10
Branches 2824 2826 +2
==========================================
+ Hits 15828 15838 +10
Misses 4785 4785
Partials 1122 1122 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
ddaspit
left a comment
There was a problem hiding this comment.
It would be good to add a unit test that covers this issue.
@ddaspit reviewed 6 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93).
src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 309 at r2 (raw file):
ScrVers? previousVersification = null; foreach (ScrVers versification in projectVersifications.Values)
Nit: The loop adds a diagnostic for every adjacent pair that is not equivalent, so it can emit more than one. For example, three projects using English, Original and English would produce two identical diagnostics. Would it be possible to compare each versification to the first one and add a single diagnostic if any of them are not equivalent?
8978ea4 to
89f5637
Compare
Enkidu93
left a comment
There was a problem hiding this comment.
Done.
@Enkidu93 made 2 comments and resolved 1 discussion.
Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on ddaspit and pmachapman).
src/Machine/src/Serval.Machine.Shared/Services/PreprocessBuildJob.cs line 309 at r2 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Nit: The loop adds a diagnostic for every adjacent pair that is not equivalent, so it can emit more than one. For example, three projects using English, Original and English would produce two identical diagnostics. Would it be possible to compare each versification to the first one and add a single diagnostic if any of them are not equivalent?
Done.
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
…le versifications attested that are not equivalent
89f5637 to
a5ee275
Compare
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 4 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Enkidu93).
…le versifications attested that are not equivalent
637b4f5 to
c210fc0
Compare
…b.com/sillsdev/serval into fix-multiple-versification-diagnostic
Requires sillsdev/machine#525.
Fixes #1043.
This change is