Port key terms updates

[Enkidu93]

Port sillsdev/machine.py#257

---

This change is 

[Enkidu93]

Fixes #383

[Enkidu93]

Why is there not a code coverage comment here? 🤔

[ddaspit]

@ddaspit reviewed 21 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Enkidu93).

---

src/SIL.Machine/Corpora/ParallelTextRow.cs line 46 at r1 (raw file):

        public TextRowFlags TargetFlags { get; set; } = TextRowFlags.SentenceStart;

        private readonly TextRowContentType _contentType;

The backing field shouldn't be necessary.

---

src/SIL.Machine/Corpora/ParatextProjectTermsParserBase.cs line 119 at r1 (raw file):

                    .Descendants()
                    .Where(n => n.Name.LocalName == "TermRendering")
                    .Where(ele => ele.Attribute("Guess") == null || ele.Attribute("Guess").Value == "false")

You can do something like this instead ((string)ele.Attribute("Guess") ?? "false") == "false". There are some other places in this class where you can use the same syntax.

---

src/SIL.Machine/Corpora/NParallelTextRow.cs line 41 at r1 (raw file):

        public IReadOnlyList<TextRowFlags> NFlags { get; set; }

        private readonly TextRowContentType _contentType;

The backing field shouldn't be necessary.

---

src/SIL.Machine/Corpora/IRow.cs line 9 at r1 (raw file):

        bool IsEmpty { get; }

        TextRowContentType ContentType { get; }

This property only applies to TextRows. We should remove it from this interface.

---

src/SIL.Machine/Corpora/TextRow.cs line 28 at r1 (raw file):

        public object Ref { get; }

        private readonly TextRowContentType _contentType;

This backing field isn't necessary.

---

src/SIL.Machine/Corpora/KeyTerm.cs line 17 at r1 (raw file):

        string category,
        string domain,
        IReadOnlyList<string> renderings,

Nit: Generally, it is better to pass in an IEnumerable to an immutable class and then copy the collection when setting to ensure that it cannot be changed. If you make the change, don't worry about updating the Python code.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 80.54299% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.75%. Comparing base (611fbf2) to head (7a3c968).

Files with missing lines	Patch %	Lines
....Machine/Corpora/ParatextProjectTermsParserBase.cs	80.95%	13 Missing and 7 partials ⚠️
src/SIL.Machine/Corpora/TextFileTextCorpus.cs	66.66%	4 Missing and 2 partials ⚠️
src/SIL.Machine/Corpora/UsfmTextBase.cs	33.33%	4 Missing and 2 partials ⚠️
src/SIL.Machine/Corpora/KeyTerm.cs	81.81%	4 Missing ⚠️
src/SIL.Machine/Corpora/NParallelTextRow.cs	63.63%	4 Missing ⚠️
....Machine/Corpora/FileParatextProjectFileHandler.cs	92.30%	0 Missing and 1 partial ⚠️
src/SIL.Machine/Corpora/ParallelTextRow.cs	88.88%	0 Missing and 1 partial ⚠️
src/SIL.Machine/Corpora/TextFileAlignmentCorpus.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
+ Coverage   72.73%   72.75%   +0.01%     
==========================================
  Files         423      424       +1     
  Lines       36029    36153     +124     
  Branches     4969     4991      +22     
==========================================
+ Hits        26205    26302      +97     
- Misses       8733     8750      +17     
- Partials     1091     1101      +10     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Enkidu93]

@Enkidu93 made 6 comments.
Reviewable status: 14 of 21 files reviewed, 6 unresolved discussions (waiting on @ddaspit).

---

src/SIL.Machine/Corpora/KeyTerm.cs line 17 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Nit: Generally, it is better to pass in an IEnumerable to an immutable class and then copy the collection when setting to ensure that it cannot be changed. If you make the change, don't worry about updating the Python code.

Makes sense. Is that what you mean? ToArray() should shallow copy, right?

---

src/SIL.Machine/Corpora/NParallelTextRow.cs line 41 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The backing field shouldn't be necessary.

Done.

---

src/SIL.Machine/Corpora/ParallelTextRow.cs line 46 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The backing field shouldn't be necessary.

Done.

---

src/SIL.Machine/Corpora/ParatextProjectTermsParserBase.cs line 119 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You can do something like this instead ((string)ele.Attribute("Guess") ?? "false") == "false". There are some other places in this class where you can use the same syntax.

I changed it here, but didn't see any other instances quite like this. Was there another spot you had in mind? Are you saying more generally that I should cast and then use ?? with a default rather than accessing .Value?

---

src/SIL.Machine/Corpora/TextRow.cs line 28 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This backing field isn't necessary.

Done.

---

src/SIL.Machine/Corpora/IRow.cs line 9 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This property only applies to TextRows. We should remove it from this interface.

Yeah, I wasn't sure about this - or whether we should create an interface for text rows. Done.

[ddaspit]

@ddaspit reviewed 7 files and all commit messages, made 2 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93).

---

src/SIL.Machine/Corpora/KeyTerm.cs line 17 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Makes sense. Is that what you mean? ToArray() should shallow copy, right?

Yes, what you have is perfect.

---

src/SIL.Machine/Corpora/ParatextProjectTermsParserBase.cs line 119 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I changed it here, but didn't see any other instances quite like this. Was there another spot you had in mind? Are you saying more generally that I should cast and then use ?? with a default rather than accessing .Value?

For values that might not exist, casting is generally preferred over the Value property.

[Enkidu93]

@Enkidu93 made 2 comments.
Reviewable status: 20 of 21 files reviewed, 1 unresolved discussion (waiting on @ddaspit).

---

src/SIL.Machine/Corpora/KeyTerm.cs line 17 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yes, what you have is perfect.

Sweet 👍

---

src/SIL.Machine/Corpora/ParatextProjectTermsParserBase.cs line 119 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

For values that might not exist, casting is generally preferred over the Value property.

That makes sense! Done.

[ddaspit]

@ddaspit reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93).