Add a PAPI data provider that reads scripture using ParatextData

[lyonsil]

This change is 

[tjcouch-sil]

How exciting!! Looking forward to having this in.

Sorry I forgot to think about this before so we could talk about design, but I lean against making this special external types extension. I think I would prefer if we made an actual extension and used the normal build process instead of adding special code for this types file. Maybe we could go for an extension that gives types for all the C# stuff, or we could go for many extensions that are split up (it is likely that people will not want all the functionality from the C# data provider one day once we provide ways to activate and deactivate extensions, so it may be best to split them up somewhat by feature). I guess it would be worth adding a special thing in extension.service.ts that checks if the manifest has a main provided at all and ignores it if not? Or we could add a boolean in the manifest like "onlyTypes" or something that indicates the extension only provides types (so it is clearer that you must provide either main or onlyTypes and we can error otherwise)? Other people may want to provide TS types that correspond to external code, so it would be good to figure out how we want to do this and make it more official.

It is also likely that there will be some JS code that corresponds to external code in some cases, and it may be that people want to join those together in one extension. Like a FLEx extension that has a JS webview or something. Just a thought for considering how we want to do this.

Reviewed 3 of 18 files at r1, all commit messages.
Reviewable status: 3 of 18 files reviewed, all discussions resolved

[lyonsil]

I think I would prefer if we made an actual extension and used the normal build process instead of adding special code for this types file.

I'm not sure what an "actual extension" would be in this case. Maybe you just mean the "extension with no code" you talk about later. If there are cases where an extension is sort of a combination of C# code and JS/TS code, then for sure I think it makes sense to have an actual extension there. In cases where there is no JS/TS code needed to call the extension, though, I would think we want some way to expose types.

Would it be reasonable to go with this current approach in this PR for now and have a follow up ticket to redesign how to expose types in TS for non-TS data providers? I feel like there could be a lot of back and forth about this point alone, and Tim might want to chime in. I'd like to get this merged soon so the extensions team has a clear way to load USFM data to work with.

Reviewable status: 3 of 18 files reviewed, all discussions resolved (waiting on @tjcouch-sil)

[irahopkinson]

I get this error on the terminal in Windows:

Reviewed 16 of 18 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lyonsil)

---

cspell.json line 46 at r3 (raw file):

    "unsubscribers",
    "usfm",
    "versificaiton"

sp

---

c-sharp/NetworkObjects/UsfmDataProvider.cs line 10 at r3 (raw file):

namespace Paranext.DataProvider.NetworkObjects
{
    // TODO: Add versificaiton support

BTW sp

Code quote:

versificaiton 

---

c-sharp/ParatextUtils/ParatextGlobals.cs line 12 at r3 (raw file):

        private static bool s_initialized = false;

        public static void Initialize(string dataFolder)

BTW I'm guessing dataFolder is a path - how about renaming to dataFolderPath to clarify. This would apply elsewhere also.

---

c-sharp/ParatextUtils/ParatextGlobals.cs line 20 at r3 (raw file):

            lock (s_locker)
            {
                if (s_initialized)

BTW could this check be outside the lock?

---

c-sharp-tests/ParatextDataConnectionTests.cs line 54 at r2 (raw file):

            ScrText scrText = ScrTextCollection.Find("WEB");
            Assert.Multiple(() =>

BTW curious why you made this multiple? Seems like failing on the first error in the order given would be appropriate?

[lyonsil]

Reviewable status: 9 of 23 files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

cspell.json line 46 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

sp

Indeed - fixed now

---

c-sharp/NetworkObjects/UsfmDataProvider.cs line 10 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW sp

Yep, this was the original problem - fixed

---

c-sharp/ParatextUtils/ParatextGlobals.cs line 20 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW could this check be outside the lock?

Moving it outside the lock introduces a small race condition. I could check outside the lock, too, which is a reasonable pattern people follow: https://en.wikipedia.org/wiki/Double-checked_locking.

It really shouldn't matter for what we're doing here.

[lyonsil]

This error is fixed in the latest changes.

Reviewable status: 8 of 23 files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/ParatextUtils/ParatextGlobals.cs line 12 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I'm guessing dataFolder is a path - how about renaming to dataFolderPath to clarify. This would apply elsewhere also.

It's a relative path, but in this case we're using something in the same directory (i.e., "assets"). Updated the name.

---

c-sharp/ParatextUtils/ParatextGlobals.cs line 20 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Moving it outside the lock introduces a small race condition. I could check outside the lock, too, which is a reasonable pattern people follow: https://en.wikipedia.org/wiki/Double-checked_locking.

It really shouldn't matter for what we're doing here.

I went ahead and added the second check.

---

c-sharp-tests/ParatextDataConnectionTests.cs line 54 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW curious why you made this multiple? Seems like failing on the first error in the order given would be appropriate?

I made this change based on Visual Studio's prompting. Here is the URL with more details on the recommendation/warning I saw: https://github.com/nunit/nunit.analyzers/blob/master/documentation/NUnit2045.md

Basically I believe the idea is that it can be helpful to see everything that failed instead of seeing one issue, fixing it, then not realizing there is another failure right after it that might require a different or additional fix.

[irahopkinson]

Yep thanks, working for me now on Windows too. FYI I can't get the 2x hello world tabs to open on Linux but I don't think that is due to this PR. I get a few TypeError: fetchOriginal is not a function errors. How do you get them showing up?

Reviewed 13 of 14 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil)

---

c-sharp/ParatextUtils/ParatextGlobals.cs line 20 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I went ahead and added the second check.

Right, this is trickier than it looks at first with the lock.

---

c-sharp-tests/ParatextDataConnectionTests.cs line 54 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I made this change based on Visual Studio's prompting. Here is the URL with more details on the recommendation/warning I saw: https://github.com/nunit/nunit.analyzers/blob/master/documentation/NUnit2045.md

Basically I believe the idea is that it can be helpful to see everything that failed instead of seeing one issue, fixing it, then not realizing there is another failure right after it that might require a different or additional fix.

Yes, I had looked that up too since I wasn't familiar with it. I guess it would make more sense to me to use the multiple on the last 2 and move the Assert.That(scrText, Is.Not.Null); before it since the last 2 won't have any meaning if scrText is null.

---

extensions/lib/external-usfm-data-provider/index.d.ts line 1 at r5 (raw file):

declare module 'external-usfm-data-provider' {}

BTW as far as I can see this file isn't really doing anything useful since the declaration is an empty object. However, if it exported items from the adjacent ./usfm-data-provider.d.ts file then the import in hello-world.web-view.tsx could be simplified from import { type UsfmProviderDataTypes } from '@extensions/external-usfm-data-provider/usfm-data-provider'; to import { type UsfmProviderDataTypes } from '@extensions/external-usfm-data-provider';.

---

extensions/vite/vite.config.ts line 77 at r5 (raw file):

      viteStaticCopy({
        targets: externalExtensions.flatMap<Target>((extension): Target => {
          return {

BTW since this only returns a single object without any processing, you can keep this format cleaner by following the example above that uses round brackets so you don't need the extra {...} and the return inside. So it would look like this:

      viteStaticCopy({
        targets: externalExtensions.flatMap<Target>(
          (extension): Target => ({
            src: `${sourceFolder}/${extension}/*.*`,
            dest: `${extension}`,
          }),
        ),
      }),

[lyonsil]

Hmm. Do you mean the "Hello World React" and "Hello World HTML" tabs? Perhaps there is another tab you're talking about that I've never seen? I don't notice anything wrong, but it might be because I always use Linux and didn't know I was missing anything. FWIW I didn't notice anything different between Windows and Linux when I was testing this PR.

Reviewable status: 21 of 23 files reviewed, all discussions resolved (waiting on @irahopkinson)

---

c-sharp-tests/ParatextDataConnectionTests.cs line 54 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Yes, I had looked that up too since I wasn't familiar with it. I guess it would make more sense to me to use the multiple on the last 2 and move the Assert.That(scrText, Is.Not.Null); before it since the last 2 won't have any meaning if scrText is null.

I agree these could be refactored as you suggest, but the current form provides the most information when the test fails. If the first assert fails I would expect all 3 to fail, but if for some crazy reason that wasn't the case, I'd like to know that sooner rather than later. It seems simpler to just leave them together and get the most information on a failure, doesn't it? Why make the code more complex and lose information?

---

extensions/vite/vite.config.ts line 77 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW since this only returns a single object without any processing, you can keep this format cleaner by following the example above that uses round brackets so you don't need the extra {...} and the return inside. So it would look like this:

      viteStaticCopy({
        targets: externalExtensions.flatMap<Target>(
          (extension): Target => ({
            src: `${sourceFolder}/${extension}/*.*`,
            dest: `${extension}`,
          }),
        ),
      }),

I think "cleaner" is in the eye of the beholder, here. 😄 Since I was removing the index file I made this change, too.

---

extensions/lib/external-usfm-data-provider/index.d.ts line 1 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW as far as I can see this file isn't really doing anything useful since the declaration is an empty object. However, if it exported items from the adjacent ./usfm-data-provider.d.ts file then the import in hello-world.web-view.tsx could be simplified from import { type UsfmProviderDataTypes } from '@extensions/external-usfm-data-provider/usfm-data-provider'; to import { type UsfmProviderDataTypes } from '@extensions/external-usfm-data-provider';.

At one point I had to add this declaration to get past a build issue. I just tried again, and it doesn't seem like it does anything now as you say. It must have been some intermediate issue that got resolved later when I made other changes. So I just removed this file.

[irahopkinson]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)

[irahopkinson]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)

[tjcouch-sil]

@lyonsil Yes, when I said at the top to make an actual extension, I did mean the same as below where I said basically an extension with no code but that just serves types.

Sure, we can split off the work into a new issue. Good suggestion! :) #267 thanks for considering this with me.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)

[FoolRunning]

Reviewed all commit messages.
Reviewable status: 22 of 23 files reviewed, all discussions resolved (waiting on @irahopkinson)

[jwickberg]

Seems you would want to use FindById to get the project - which would mean that you would need to have id as part of the arguments. Name isn't guaranteed to be unique.

Also, you later give an error about StartDataProvider not being called based on _scrText being null, but the Find and FindById methods will return null if the project isn't found. You probably want a separate error for that.

[lyonsil]

For the moment we're loading out of fixed data files checked into the repo. Once we start working with "real" projects with multiple collections, this will definitely need to be reworked. There isn't a design in play yet for how we will work with projects, so I went with something very simple.

[lyonsil]

Dismissed @jwickberg from a discussion.
Reviewable status: 22 of 23 files reviewed, 1 unresolved discussion (waiting on @FoolRunning, @irahopkinson, @jwickberg, and @tjcouch-sil)

[irahopkinson]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FoolRunning, @jwickberg, and @tjcouch-sil)