-
-
Notifications
You must be signed in to change notification settings - Fork 3
Standardize draft generation terminology #37
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
base: main
Are you sure you want to change the base?
Conversation
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Nathaniel! Thanks for jumping in and working on this! I have some feedback for you pretty much just around avoiding breaking changes, but it looks good overall!
@tjcouch-sil reviewed 5 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson, @lyonsil, and @Nateowami).
src/scripture-forge/src/types/scripture-forge.d.ts line 451 at r1 (raw file):
* @returns WebView id for new Scripture Forge home WebView or `undefined` if not created */ 'scriptureForge.openGeneratedDrafts': () => Promise<string | undefined>;
To avoid breaking changes (e.g. to avoid another extension calling the old name and failing), please deprecate the existing command and make this new command.
src/scripture-forge/src/main.ts line 236 at r1 (raw file):
const openGeneratedDraftsCommandPromise = papi.commands.registerCommand( 'scriptureForge.openGeneratedDrafts', async () => {
To avoid breaking changes as mentioned in the type declaration file, you can register this same function twice for the two different command names.
src/scripture-forge/contributions/menus.json line 8 at r1 (raw file):
{ "label": "%mainMenu_scriptureForge_openGeneratedDrafts_label%", "localizeNotes": "Application main menu > Project > Open Generated Drafts",
I see the casing is different for "Open Generated Drafts" here vs "Open generated drafts" in the localized string. Which one is the expected capitalization? Just double checking that the localized string capitalization is right since it is different in these two places; it's not actually essential for these localizeNotes to have the right capitalization ;)
IIRC UX says to use sentence case for menu items in general, but I dunno if "Generated Drafts" is a proper noun that would warrant title case like I did with "Auto Drafts" before.
src/scripture-forge/contributions/localizedStrings.json line 5 at r1 (raw file):
"localizedStrings": { "en": { "%mainMenu_scriptureForge_openGeneratedDrafts_label%": "Open draft generation",
Regarding all these localized strings, please deprecate and make new strings instead of changing or renaming existing ones to avoid breaking changes (e.g. another extension using one of these changed strings; unlikely but possible) since this extension has been published and distributed with these strings.
3b7d125 to
5fd8e8a
Compare
Nateowami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
@Nateowami made 6 comments.
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil).
src/scripture-forge/contributions/localizedStrings.json line 5 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Regarding all these localized strings, please deprecate and make new strings instead of changing or renaming existing ones to avoid breaking changes (e.g. another extension using one of these changed strings; unlikely but possible) since this extension has been published and distributed with these strings.
I can't tell from the linked documentation how to deprecate a string. The file has this:
"%test_deprecated_string%": {
"deprecationInfo": {
"date": "2025-04-21",
"message": "This exists to test deprecating localized strings. Do not remove or use."
}
},I see an object, but not the original string. In this file the format is "key": "string", but there the format seems to be "key": <object> with no string.
src/scripture-forge/contributions/menus.json line 8 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
I see the casing is different for "Open Generated Drafts" here vs "Open generated drafts" in the localized string. Which one is the expected capitalization? Just double checking that the localized string capitalization is right since it is different in these two places; it's not actually essential for these
localizeNotesto have the right capitalization ;)IIRC UX says to use sentence case for menu items in general, but I dunno if "Generated Drafts" is a proper noun that would warrant title case like I did with "Auto Drafts" before.
I depends on the application conventions. For the most part I tried to keep the same casing unless I thought it was obviously incorrect. I'll change it.
src/scripture-forge/src/main.ts line 236 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
To avoid breaking changes as mentioned in the type declaration file, you can register this same function twice for the two different command names.
Done.
src/scripture-forge/src/types/scripture-forge.d.ts line 451 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
To avoid breaking changes (e.g. to avoid another extension calling the old name and failing), please deprecate the existing command and make this new command.
Done.
src/scripture-forge/contributions/localizedStrings.json line 10 at r1 (raw file):
"%project_settings_scriptureForge_projectId_description%": "This project's ID according to Scripture Forge. It is generated upon connecting a project to Scripture Forge. This is distinct from the project's ID according to Paratext. Empty means the project is not connected to Scripture Forge.", "%scriptureForge_draft_action_canJoin%": "Join project", "%scriptureForge_draft_action_cannotAccessDrafts%": "Ask your administrator for access to generate drafts.",
How do I handle this one? The string should change, but the key still makes sense. Same question for the changes lower in this file.
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjcouch-sil reviewed 4 files and all commit messages, made 5 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @irahopkinson, @lyonsil, and @Nateowami).
src/scripture-forge/contributions/localizedStrings.json line 5 at r1 (raw file):
Previously, Nateowami wrote…
I can't tell from the linked documentation how to deprecate a string. The file has this:
"%test_deprecated_string%": { "deprecationInfo": { "date": "2025-04-21", "message": "This exists to test deprecating localized strings. Do not remove or use." } },I see an object, but not the original string. In this file the format is
"key": "string", but there the format seems to be"key": <object>with no string.
Ah, I see; the example linked was from core code, not extension code. Sorry. I updated the example there. Here's the new extension code link for convenience.
src/scripture-forge/contributions/localizedStrings.json line 10 at r1 (raw file):
Previously, Nateowami wrote…
How do I handle this one? The string should change, but the key still makes sense. Same question for the changes lower in this file.
However you want :) I think we often just add a _2 to the end, but I don't think we really have a convention.
src/scripture-forge/contributions/menus.json line 8 at r1 (raw file):
Previously, Nateowami wrote…
I depends on the application conventions. For the most part I tried to keep the same casing unless I thought it was obviously incorrect. I'll change it.
Gotcha, thanks!
src/scripture-forge/src/main.ts line 236 at r1 (raw file):
Previously, Nateowami wrote…
Done.
Thanks!
src/scripture-forge/src/types/scripture-forge.d.ts line 451 at r1 (raw file):
Previously, Nateowami wrote…
Done.
Thanks! Sorry; something I didn't think to mention is that we always put the deprecation date on our deprecation messages in addition to the suggestions for how to adapt (like you already put). We intend to support deprecated things for some X amount of time (we haven't decided yet) before removing them, so this makes it easy for us and extension developers to keep track of when something will be removed (at least, once we decide on the deprecation time limit! 😂)
5fd8e8a to
5e70076
Compare
5e70076 to
ab3b1dc
Compare
tjcouch-sil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than one thing to check in on. Thanks! I appreciate your patience with our processes :)
@tjcouch-sil reviewed 3 files and all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson, @lyonsil, and @Nateowami).
src/scripture-forge/contributions/localizedStrings.json line 43 at r3 (raw file):
"%scriptureForge_draft_action_canJoin%": "Join project", "%scriptureForge_draft_action_cannotAccessDrafts%": "Ask your administrator for access to Auto Drafts.", "%scriptureForge_draft_action_cannotAccessDrafts_2%": "Ask your administrator for access to generate drafts.",
These newly changed localized string keys need to have their usage updated, right?
If I missed something, feel free to resolve this and merge :)
This PR updates the Scripture Forge extension to consistently use the term "Generated Drafts" or "Draft Generation" instead of "Auto Drafts" throughout the codebase. This aligns the extension with the terminology Scripture Forge and the teams working with it use. "Auto drafts" was in the Scripture Forge UI in a few places for a short while before we had agreed on terminology, but was never an agreed-upon term.
This change is