Reworked ParatextStandard PDP to include USFM in data type names, wrote TypeScript types for potential Scripture and Project Note data #538
Conversation
…te TypeScript types for potential Scripture and Project Note data
There was a problem hiding this comment.
Looks good. I don't know if I love USJ, but it's something to move the conversation along, and it's a spec we didn't have to write! 😄
I just have one small doc update I think would be good to add.
Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)
extensions/src/project-notes-data-provider/index.d.ts line 17 at r1 (raw file):
/** Data provider for manipulating project notes */ export type ProjectNotesDataProvider = IDataProvider<ProjectNotesProviderDataTypes> & {
Can you add a pointer of some kind back to the PT9 source/docs where this came from? Also, it's probably good to call out this was put together to be compatible with PT9, so any suggested changes need to maintain that compatibility.
extensions/src/usfm-data-provider/index.d.ts line 29 at r1 (raw file):
export type ParatextStandardProjectDataTypes = MandatoryProjectDataType & { /** Gets the "raw" USFM data for the specified book */ BookUSFM: DataProviderDataType<VerseRef, string | undefined, string>;
Looks like Intelliense doesn't pick up comments on data provider methods in particular, so these comments are only for other reviewers. I think we'd need to get getProjectDataProvider to return these specifically more detailed types for the comments to show up. It shouldn't block this PR from going in, but it's something to keep in mind.
There was a problem hiding this comment.
Reviewed 2 of 8 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)
There was a problem hiding this comment.
Yeah, I emailed Martin Hoskin with a ton of feedback Friday (because he asked haha). Hopefully that will lead to a better spec. I bet he would love to hear your feedback as well. However, it might be good to discuss together so we don't send duplicate feedback.
BTW the official proposal and a few discussions are here usfm-bible/tcdocs#42
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @lyonsil)
extensions/src/project-notes-data-provider/index.d.ts line 17 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Can you add a pointer of some kind back to the PT9 source/docs where this came from? Also, it's probably good to call out this was put together to be compatible with PT9, so any suggested changes need to maintain that compatibility.
Good idea. Done
extensions/src/usfm-data-provider/index.d.ts line 29 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Looks like Intelliense doesn't pick up comments on data provider methods in particular, so these comments are only for other reviewers. I think we'd need to get
getProjectDataProviderto return these specifically more detailed types for the comments to show up. It shouldn't block this PR from going in, but it's something to keep in mind.
Agreed. I have been thinking it would probably be best to make a mapped type for the PDPs themselves (hopefully we could infer these PDPDataType types from that instead of having two shared interfaces, but I seemed to have some issues with infer when I was initially creating the DataProvider types long ago). Issue here #517
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 6 of 8 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
Related to #479, #480.
Quick and dirty proposed types for Scripture and Project Notes data providers so the Information Architecture group can read them and get an idea of what's going on. There are many outstanding questions on Project Notes as I essentially copied them straight from the PT9 plugin interface. We still don't know what tokenization of USFM we will use, so I started with USJ.
This change is