Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a service to lookup projects on the file system #476

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

lyonsil
Copy link
Member

@lyonsil lyonsil commented Sep 28, 2023

This also removes all the fake project tests that were included since those fake projects don't exist in the lookup service.

Once we have some way to populate all dev environments with at least 1 default project, we will probably remove the USFM data provider and replace it with a project data provider.


This change is Reviewable

Copy link
Member Author

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have some project under <user_home>/.platform.bible/projects/<name>_<id>, with a project metadata file and project contents in place, the following code should work from an extension:

  const projects = await papi.projectLookup.getMetadataForAllProjects();
  const pdp = await papi.projectDataProvider.getProjectDataProvider<'ParatextStandard'>(projects[0].id);
  const verse = await pdp.getVerse(new VerseRef('JHN', '1', '1'));
  logger.info(`Got verse from PDP: ${verse}`);

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

@lyonsil lyonsil linked an issue Sep 28, 2023 that may be closed by this pull request
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow!! :) soooooo excited for where this is headed. Thanks for all the great work!

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)


src/extension-host/services/project-lookup-backend.service.ts line 13 at r1 (raw file):

import * as nodeFS from '@node/services/node-file-system.service';

const PROJECTS_ROOT_URI = joinUriPaths('file://', os.homedir(), '.platform.bible', 'projects');

Think we should make a scheme out of this? projects:// or something? I realize now that it is not within the data:// scheme. Or maybe we should put it in the data scheme...?


src/shared/models/project-metadata.model.ts line 1 at r1 (raw file):

export type ProjectMetadata = {

Can we add a JSDoc to this? Maybe something like the following:

Low-level information describing a project that Platform.Bible directly manages and uses to load project data


src/shared/services/project-data-provider.service.ts line 105 at r1 (raw file):

): Promise<ProjectDataProvider[ProjectType]> {
  // Get the network object backing the project lookup service because that works differently on
  // the front end and back end through PAPI, and this looks the same.

I was under the impression that we would use the "client" service everywhere in the new server/client service model. Is it possible to import service from the project-lookup.service.ts in both extension host and everywhere else? For example, I see in papi-backend.service.ts that it imports from that file, not from the project-lookup-backend.service.ts, right?

Copy link
Member Author

@lyonsil lyonsil 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: 10 of 13 files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)


src/extension-host/services/project-lookup-backend.service.ts line 13 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Think we should make a scheme out of this? projects:// or something? I realize now that it is not within the data:// scheme. Or maybe we should put it in the data scheme...?

I thought about it, but I didn't see how making a scheme would actually simplify anything in a meaningful way. If we find we're redoing this sort of URI creation later for project files we can always add it then and update this. Do you see a compelling need to do it now?


src/shared/models/project-metadata.model.ts line 1 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Can we add a JSDoc to this? Maybe something like the following:

Low-level information describing a project that Platform.Bible directly manages and uses to load project data

Sounds great - done


src/shared/services/project-data-provider.service.ts line 105 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I was under the impression that we would use the "client" service everywhere in the new server/client service model. Is it possible to import service from the project-lookup.service.ts in both extension host and everywhere else? For example, I see in papi-backend.service.ts that it imports from that file, not from the project-lookup-backend.service.ts, right?

Yes, you are totally right. Actually after I wrote these few lines of code I remembered that's how we left that discussion, and I went back and created the "client" service (ProjectLookupService). After doing all that work, though, I forgot to go back here and actually use it. 🤦

Fixed now.

tjcouch-sil
tjcouch-sil previously approved these changes Sep 29, 2023
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: thank you!! Just a bit of non-blocking conversation left. If you don't want to do anything more, that's understandable.

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)


src/extension-host/services/project-lookup-backend.service.ts line 13 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I thought about it, but I didn't see how making a scheme would actually simplify anything in a meaningful way. If we find we're redoing this sort of URI creation later for project files we can always add it then and update this. Do you see a compelling need to do it now?

Seems reasonable not to make a scheme just for this. Actually I just realized you would have to do some extra work to support a custom scheme on the C# side: In development, our custom schemes point not to the actual home folder but to paranext-core/dev-appdata, so you would need to adjust the C# side accordingly. We're not even passing --isPackaged to the dotnet process right now, so maybe this belongs in another issue if we think it makes sense to do (separate the dev projects folder from the prod folder like we do with all our other data).

Would you mind adding a comment to this line that mentions that this path always points to the home directory project folder unlike when we use our custom schemes? I didn't even catch the difference til just now, and I think that could be confusing for people.

I guess then the only other thing would be determining if it's worth putting stuff in the data folder (where the data:// scheme points but not dependent on dev/prod like I mentioned above) like ~/.platform.bible/data/projects. I think I would prefer to put projects in there if you think that's sensible. Doesn't matter a lot; maybe we say that folder is just for user data other than projects. Let me know your thoughts.


src/shared/services/project-data-provider.service.ts line 105 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Yes, you are totally right. Actually after I wrote these few lines of code I remembered that's how we left that discussion, and I went back and created the "client" service (ProjectLookupService). After doing all that work, though, I forgot to go back here and actually use it. 🤦

Fixed now.

Gotcha! Thanks for the quick fix!

Copy link
Member Author

@lyonsil lyonsil 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: 12 of 13 files reviewed, all discussions resolved (waiting on @tjcouch-sil)


src/extension-host/services/project-lookup-backend.service.ts line 13 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Seems reasonable not to make a scheme just for this. Actually I just realized you would have to do some extra work to support a custom scheme on the C# side: In development, our custom schemes point not to the actual home folder but to paranext-core/dev-appdata, so you would need to adjust the C# side accordingly. We're not even passing --isPackaged to the dotnet process right now, so maybe this belongs in another issue if we think it makes sense to do (separate the dev projects folder from the prod folder like we do with all our other data).

Would you mind adding a comment to this line that mentions that this path always points to the home directory project folder unlike when we use our custom schemes? I didn't even catch the difference til just now, and I think that could be confusing for people.

I guess then the only other thing would be determining if it's worth putting stuff in the data folder (where the data:// scheme points but not dependent on dev/prod like I mentioned above) like ~/.platform.bible/data/projects. I think I would prefer to put projects in there if you think that's sensible. Doesn't matter a lot; maybe we say that folder is just for user data other than projects. Let me know your thoughts.

Comment added

I think it's reasonable for projects to live where we have them now. I don't think it's that big of a deal either way, but changing at this point would mean changing code and specs as they all mention ~/.platform.bible/projects as the place. So it's probably not worth changing unless there is something special that will be done with the data directory in some other way.

Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lyonsil lyonsil merged commit 2270dd5 into main Sep 29, 2023
6 checks passed
@lyonsil lyonsil deleted the 357-get-available-projects branch September 29, 2023 16:51
irahopkinson added a commit that referenced this pull request Oct 1, 2023
- fix changelog
- PR #476 - style: vars start with lowercase names
@irahopkinson irahopkinson mentioned this pull request Oct 1, 2023
irahopkinson added a commit that referenced this pull request Oct 2, 2023
- fix changelog
- PR #476 - style: vars start with lowercase names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Projects: Get list of available project IDs and their metadata
2 participants