Skip to content

Conversation

@pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Dec 8, 2025

The SourceQuoteConvention property is marked obsolete in Serval 1.12, and is populated with the value "ignore", rather than calculated.

This PR removes that property, as it is no longer necessary.


This change is Reviewable

@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.78%. Comparing base (b78d54a) to head (46a2113).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3607      +/-   ##
==========================================
- Coverage   82.78%   82.78%   -0.01%     
==========================================
  Files         610      610              
  Lines       37239    37237       -2     
  Branches     6100     6105       +5     
==========================================
- Hits        30830    30828       -2     
  Misses       5481     5481              
  Partials      928      928              

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

Copy link
Collaborator

@Nateowami Nateowami 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 2 files reviewed, 1 unresolved discussion (waiting on @pmachapman)


src/SIL.XForge.Scripture/Services/MachineApiService.cs line 2292 at r1 (raw file):

                    && translationBuild.Analysis?.FirstOrDefault(a =>
                        a.ParallelCorpusRef == parallelCorpusId
                        && !string.IsNullOrEmpty(a.SourceQuoteConvention)

This looks to me like it is duplicating #3601, except not going as far. Am I missing something?

@pmachapman pmachapman force-pushed the fix/remove-source-quote-convention branch from 4f52e2f to 46a2113 Compare December 8, 2025 22:02
@pmachapman
Copy link
Collaborator Author

@Nateowami Yes, you are right. sorry I didn't see your PR.

@pmachapman pmachapman closed this Dec 8, 2025
@Nateowami
Copy link
Collaborator

No problem; I'd rather multiple people be aware of a need for the same improvement than something get completely ignored. Should I update my PR to remove SourceQuoteConvention from the tests?

@pmachapman
Copy link
Collaborator Author

Should I update my PR to remove SourceQuoteConvention from the tests?

Yes, the property just gets populated with "ignore", and will one day be removed.

@Nateowami
Copy link
Collaborator

Actually looks like I removed SourceQuoteConvention in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants