Support daughter Paratext projects

[Enkidu93]

Also, port sillsdev/machine#381.

Related to sillsdev/serval#854 and sillsdev/silnlp#929.

---

This change is 

[Enkidu93]

Needed for sillsdev/silnlp#946

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.35028% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.49%. Comparing base (3ada08b) to head (0a32916).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
machine/corpora/paratext_backup_text_corpus.py	62.50%	3 Missing ⚠️
machine/corpora/paratext_project_settings.py	90.47%	2 Missing ⚠️
machine/corpora/paratext_text_corpus.py	71.42%	2 Missing ⚠️
...chine/corpora/zip_paratext_project_text_updater.py	75.00%	1 Missing ⚠️
...ora/zip_paratext_project_versification_detector.py	75.00%	1 Missing ⚠️
.../zip_paratext_project_quote_convention_detector.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
+ Coverage   90.75%   91.49%   +0.73%     
==========================================
  Files         355      355              
  Lines       22677    22801     +124     
==========================================
+ Hits        20581    20862     +281     
+ Misses       2096     1939     -157     

☔ 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.

[ddaspit]

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

---

machine/corpora/paratext_project_settings.py line 70 at r1 (raw file):

    @property
    def parent_has_been_set(self) -> bool:

This property and the following methods seem like implementation details of the settings parser. I don't think there is value in putting this logic here.

---

machine/corpora/paratext_project_settings_parser_base.py line 92 at r1 (raw file):

        translation_info_setting = settings_tree.getroot().findtext("TranslationInfo")
        translation_type = "Standard"

It would be nice to specify the types for these variables so that proper type checking will kick in.

---

machine/corpora/paratext_project_settings_parser_base.py line 126 at r1 (raw file):

                    f"not the parent project of project {settings.name}."
                )
            settings.set_parent_project(self.parent_paratext_project_settings)

We should just set the verification when we create the settings object.

[Enkidu93]

@Enkidu93 made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ddaspit).

---

machine/corpora/paratext_project_settings.py line 70 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This property and the following methods seem like implementation details of the settings parser. I don't think there is value in putting this logic here.

Yeah, I wasn't sure where this logic made the most sense. Have you looked at the silnlp PR? We could remove the logic here, but then we would have to replicate it in silnlp and in Serval (which, to be fair, isn't that complex). I would say if you want to remove these methods, then we might as well (besides exposing the GUIDs) not make any changes in machine.py; we can just set the versification to the parent's versification in silnlp. Our options are:

• Handle the parent-finding/setting logic in the calling code
• Handle it in this class
• Have some kind of utility class to handle it

If the setting the parent only means settings the versification, that definitely makes just letting the client do it more reasonable, but I like the re-usability we get by having it in machine. We can have shared error-checking (i.e., "That's not my parent!") and avoid writing if settings.parent_guid is not None and settings.parent_guid == candidate_parent_settings.guid: settings.versification = candidate_parent_settings.versification a half a dozen times. Look at the other PR and let me know what you think!

---

machine/corpora/paratext_project_settings_parser_base.py line 92 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

It would be nice to specify the types for these variables so that proper type checking will kick in.

I think done 😁. Let me know if you see something I missed that you'd like typed

---

machine/corpora/paratext_project_settings_parser_base.py line 126 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should just set the verification when we create the settings object.

See comment above.

[ddaspit]

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

---

machine/corpora/paratext_project_settings.py line 70 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yeah, I wasn't sure where this logic made the most sense. Have you looked at the silnlp PR? We could remove the logic here, but then we would have to replicate it in silnlp and in Serval (which, to be fair, isn't that complex). I would say if you want to remove these methods, then we might as well (besides exposing the GUIDs) not make any changes in machine.py; we can just set the versification to the parent's versification in silnlp. Our options are:

• Handle the parent-finding/setting logic in the calling code
• Handle it in this class
• Have some kind of utility class to handle it

If the setting the parent only means settings the versification, that definitely makes just letting the client do it more reasonable, but I like the re-usability we get by having it in machine. We can have shared error-checking (i.e., "That's not my parent!") and avoid writing if settings.parent_guid is not None and settings.parent_guid == candidate_parent_settings.guid: settings.versification = candidate_parent_settings.versification a half a dozen times. Look at the other PR and let me know what you think!

I looked at the silnlp PR. I can see the value in the is_daughter_project_of method, so we should keep that one. I don't think we need parent_has_been_set and set_parent_project. In any case, I think it is valuable for the settings parser to set the versification. One of the main advantages of the settings parser is that it abstracts all of the details of properly retrieving the project settings, including what settings are inherited from the parent project. Also, if silnlp isn't using the settings parser, it should.

[Enkidu93]

@Enkidu93 made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit).

---

machine/corpora/paratext_project_settings.py line 70 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I looked at the silnlp PR. I can see the value in the is_daughter_project_of method, so we should keep that one. I don't think we need parent_has_been_set and set_parent_project. In any case, I think it is valuable for the settings parser to set the versification. One of the main advantages of the settings parser is that it abstracts all of the details of properly retrieving the project settings, including what settings are inherited from the parent project. Also, if silnlp isn't using the settings parser, it should.

Ok! Yes, I agree that is_daughter_project_of is the most valuable and these properties are a little clumsy. Originally, I was thinking maybe we wanted to log a warning when relevant properties were being accessed but the parent hadn't been set. That's also because I wasn't sure if there were other attributes that should be inherited.

Looking back at this today, if we wanted to have some reference to the parent settings in the child settings, we could just add a parent property whose setter sets the versification. That would be cleaner than this. Mainly, I was thinking that including the parent would improve visibility. E.g., in the calling code or while debugging, you might ask yourself "This settings object's has_parent is True, but has the parent been properly loaded or not?" I don't like that we wouldn't have any way to tell (because maybe, say, the parent wasn't passed in to the settings parser). It would also make it a little cleaner to extend if we determine that other attributes need to be inherited from the parent; there are a lot of different types of daughter projects, and I don't think we really know whether or not we're supporting them correctly. Maybe I'm just getting ahead of my requirements 😁 . Let me know what you think.

And yes, silnlp is using the settings parser. Previously, it was not in places, but I updated it to use the settings parser in all the code I edited.

[ddaspit]

@ddaspit made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93).

---

machine/corpora/paratext_project_settings.py line 70 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Ok! Yes, I agree that is_daughter_project_of is the most valuable and these properties are a little clumsy. Originally, I was thinking maybe we wanted to log a warning when relevant properties were being accessed but the parent hadn't been set. That's also because I wasn't sure if there were other attributes that should be inherited.

Looking back at this today, if we wanted to have some reference to the parent settings in the child settings, we could just add a parent property whose setter sets the versification. That would be cleaner than this. Mainly, I was thinking that including the parent would improve visibility. E.g., in the calling code or while debugging, you might ask yourself "This settings object's has_parent is True, but has the parent been properly loaded or not?" I don't like that we wouldn't have any way to tell (because maybe, say, the parent wasn't passed in to the settings parser). It would also make it a little cleaner to extend if we determine that other attributes need to be inherited from the parent; there are a lot of different types of daughter projects, and I don't think we really know whether or not we're supporting them correctly. Maybe I'm just getting ahead of my requirements 😁 . Let me know what you think.

And yes, silnlp is using the settings parser. Previously, it was not in places, but I updated it to use the settings parser in all the code I edited.

Your proposed change to add a parent property makes sense to me. I'm happy to hear that you updated silnlp to use the settings parser.

[ddaspit]

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