Unconstrain project folder names (623)

[tombogle]

This change is 

[tombogle]

Local projects do seem to load, but I'm getting errors when trying to retrieve them from the ScrTextCollection.
When I try to show the text collection, the view just shows "Loading" (not populated with any project data), and the console shows the error: verse-display.component.tsx:15 Uncaught TypeError: papi_frontend_reactWEBPACK_IMPORTED_MODULE_0.useProjectData.VerseUSFM is not a function.
When I try to open the resource viewer, I get the error: Uncaught (in promise) Error: Paratext.Data.ProjectNotFoundException: Project not found: 621f86a6723f4800ded2cbbc41815f737732c60b (which seems more likely to be related to my change). I'm wondering if maybe that could be another incarnation of the uppercase/lowercase problem, because I see when it gets added to the Paratext project map it is uppercased. Though looking at the code in ParatextData, it seems like it should be case-agnostic, since it converts the hex string into a byte array.
I did change the (existing) code where we populate the _projectDetailsMap to add an uppercase version of the ID as the key, since GetProjectDetails(string projectId) does that, but that didn't make any difference.

[lyonsil]

I did some debugging and found the problem. If you haven't tried it, you can use a normal debugger in VS Code for our .NET Data Provider by using the "Attach to .NET Core" option on the "Run and Debug" tab. You first have to start the platform separately (I usually use "Debug Platform Backend".) Once it is running, you can switch to "Attach to .NET Core" and then click the green triangle next to the dropdown. Unfortunately there isn't a way to start the .NET process in the debugger itself easily, but for this scenario it's fine to attach it later then use the new code.

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @tombogle)

---

c-sharp/Projects/LocalProjects.cs line 138 at r1 (raw file):

            projectDetails.HomeDirectory,
            PROJECT_SUBDIRECTORY,
            projectDetails.Metadata.ProjectType

This is not "paratext". This is generally "ParatextStandard" I believe. You can't substitute one for the other as currently designed. This line should be reverted for now.

---

c-sharp/Projects/LocalProjects.cs line 149 at r1 (raw file):

        _projectDetailsMap[id.ToUpperInvariant()] = projectDetails;

        if (projectDetails.Metadata.ProjectType == ProjectMetadata.PARATEXT)

Same problem as above. Since the metadata project type doesn't match "paratext", the project never gets added to the ScrTextCollection. That is why Get fails to work later.

Until we have more project types and figure out exactly what that looks like, I would drop this if check and just add the projects to the ScrTextCollection always.

[lyonsil]

Reviewable status: 0 of 18 files reviewed, all discussions resolved

---

c-sharp/Projects/LocalProjects.cs line 138 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This is not "paratext". This is generally "ParatextStandard" I believe. You can't substitute one for the other as currently designed. This line should be reverted for now.

Fixed

---

c-sharp/Projects/LocalProjects.cs line 149 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Same problem as above. Since the metadata project type doesn't match "paratext", the project never gets added to the ScrTextCollection. That is why Get fails to work later.

Until we have more project types and figure out exactly what that looks like, I would drop this if check and just add the projects to the ScrTextCollection always.

Fixed

[lyonsil]

I picked up where Tom paused for the EMDC trip and tried to finish this off. As part of the work I tried to clarify that LocalProjects is really focused only on Paratext projects as it is written for now. Once we add more project types we can think about how to refactor it, but I feel like exactly what that should look like is outside the scope of this ticket and requires having another project type available to try it out with real examples.

Reviewable status: 0 of 18 files reviewed, all discussions resolved

[lyonsil]

Oh, and all the spacing changes were from the prettier plug-in that auto formats lines that are too long. I assume others just don't have this installed, but I'm surprised it wasn't done automatically on previous check-ins.

Reviewable status: 0 of 18 files reviewed, all discussions resolved

[irahopkinson]

one non-blocking question.

Reviewed 4 of 8 files at r1, 13 of 14 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)

---

c-sharp/Projects/LocalParatextProjects.cs line 86 at r3 (raw file):

    #region Protected properties and methods

    protected virtual string ProjectRootFolder { get; }

BTW I'm wondering if this should be at the top of the class with the other field declarations? I couldn't easily see its declaration when the constructor assigned to it.

---

c-sharp/Projects/LocalParatextProjects.cs line 108 at r3 (raw file):

    }

    protected static void CreateDirectory(string dir)

Thanks for making the code DRYer.

---

c-sharp-tests/Projects/LocalParatextProjectsTests.cs line 7 at r3 (raw file):

[ExcludeFromCodeCoverage]
[TestFixture]

FYI thanks for the improvement to the tests.

(Although strictly these are no longer unit tests because they are writting to the actual FS, whereas before I think they were still unit tests.)

[tjcouch-sil]

thanks to Tom for the great stuff and to Matt for finishing things off!

Reviewed 4 of 8 files at r1, 13 of 14 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

c-sharp/Projects/LocalParatextProjects.cs line 86 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I'm wondering if this should be at the top of the class with the other field declarations? I couldn't easily see its declaration when the constructor assigned to it.

This isn't a field, it's a property. They are similar, but not really the same. One important difference is that fields will always be assigned a value when an object is constructed, but a property may or may not. As such, fields are almost always listed next to the constructors, but not so with properties. This kind of getter property (i.e., one without a setter and without a function body) is the only kind of property that typically has an assigned value in the constructor. Getters are usually in my experience either set apart on their own or grouped together with functions inside of a file.

Prior to this change, this was a private readonly field. It was made a protected virtual for testing purposes, and the only things that are protected here are marked that way because tests want to override them or call them even though they aren't public. I thought, then, it would be easier to keep all the protected things together since tests will want to do something with them.

If it's too obscure/hidden down here, I think I would change it from a getter to a field again. It's really weird to mix properties and getters right by the constructor in my experience.

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle)

---

c-sharp/Projects/LocalParatextProjects.cs line 86 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This isn't a field, it's a property. They are similar, but not really the same. One important difference is that fields will always be assigned a value when an object is constructed, but a property may or may not. As such, fields are almost always listed next to the constructors, but not so with properties. This kind of getter property (i.e., one without a setter and without a function body) is the only kind of property that typically has an assigned value in the constructor. Getters are usually in my experience either set apart on their own or grouped together with functions inside of a file.

Prior to this change, this was a private readonly field. It was made a protected virtual for testing purposes, and the only things that are protected here are marked that way because tests want to override them or call them even though they aren't public. I thought, then, it would be easier to keep all the protected things together since tests will want to do something with them.

If it's too obscure/hidden down here, I think I would change it from a getter to a field again. It's really weird to mix properties and getters right by the constructor in my experience.

I think I'm going to merge this as-is and let @tombogle decide if he thinks it makes more sense to change back to a field. He switched it from a field to a property originally, and I don't know if he had something in mind when making that change that I'm not aware of.