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

Fix inconsistent spelling of papi namespaces #648

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

rolfheij-sil
Copy link
Contributor

@rolfheij-sil rolfheij-sil commented Nov 20, 2023

This change is Reviewable

@rolfheij-sil rolfheij-sil changed the title 320 fix inconsistent spelling of papi namespaces Fix inconsistent spelling of papi namespaces Nov 20, 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.

Awesome! Thanks for the thorough revision. This is great. Just one thing that needs to be adjusted for accuracy

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)


src/shared/services/graphql.service.ts line 67 at r1 (raw file):

  const existingPdp = paratextProjectMap.get(projectId);
  if (existingPdp) return { pdp: existingPdp, verseRef: parsedVerseRef };
  const pdp = await get<'ParatextStandard'>('ParatextStandard', projectId);

Oops! Thanks for fixing this one that I missed. Side note: you can remove the <'ParatextStandard'> generic parameter specification now that you have it in the argument. But we don't need to; just fyi for future ease of use :)


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

  if (projectType && projectType !== projectTypeFromMetadata)
    logger.warn(
      `Project type for project ${projectId} is ${projectTypeFromMetadata}, but the 'get' function on the ProjectDataProvider was run with mismatching projectType ${projectType}. This could cause issues`,

Unfortunately this is not correct. The mismatching project types here are from the PDP (projectTypeFromMetadata) and from what is put into this papi.projectDataProviders.get function as the first argument (projectType). The get function on the PDP has not been run yet. I recommend revising to something along these lines:

`Project type for project ${projectId} is ${projectTypeFromMetadata}, but `papi.projectDataProviders.get` was run with mismatching projectType ${projectType}. This could cause issues`

lyonsil
lyonsil previously approved these changes Nov 20, 2023
Copy link
Member

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

Looks good to me other than the things TJ pointed out.

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Thanks guys!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)


src/shared/services/graphql.service.ts line 67 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Oops! Thanks for fixing this one that I missed. Side note: you can remove the <'ParatextStandard'> generic parameter specification now that you have it in the argument. But we don't need to; just fyi for future ease of use :)

Done


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

Previously, tjcouch-sil (TJ Couch) wrote…

Unfortunately this is not correct. The mismatching project types here are from the PDP (projectTypeFromMetadata) and from what is put into this papi.projectDataProviders.get function as the first argument (projectType). The get function on the PDP has not been run yet. I recommend revising to something along these lines:

`Project type for project ${projectId} is ${projectTypeFromMetadata}, but `papi.projectDataProviders.get` was run with mismatching projectType ${projectType}. This could cause issues`

Done.

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:

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

@rolfheij-sil rolfheij-sil merged commit 49e6cda into main Nov 21, 2023
7 checks passed
@rolfheij-sil rolfheij-sil deleted the 320-fix-inconsistent-spelling-of-papi-namespaces branch November 21, 2023 15:48
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.

3 participants