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

feat: Add translation key prefixes for upload and download #687

Conversation

malvarezphrase
Copy link
Contributor

@malvarezphrase malvarezphrase commented Sep 24, 2024

Purpose: Provide documentation for the new params on import and export.

Pushing:

  • provide a prefix with translation_key_prefix which will be prepended to the name of the imported translation keys
  • provide <file_name> as the translation_key_prefix to use the file path of the pushed file as a prefix

Pulling:

  • provide a prefix with translation_key_prefix which will be subtracted from the name of the exported translation keys. Warning: this may create duplicate key names if other keys share the same name after the prefix is removed
  • provide <file_name> as the translation_key_prefix to use the file path of the pulled file as a prefix
  • set the option use_translation_key_prefix_as_filter to true for the prefix to work as a filter. In this case only translation keys including the provided prefix in their name will be exported

Related tickets:
https://phrase.atlassian.net/browse/STRINGS-374
https://phrase.atlassian.net/browse/STRINGS-439

@malvarezphrase malvarezphrase changed the title STRINGS-374 Enable users to provide a prefix during CLI push feat: STRINGS-374 Enable users to provide a prefix during CLI push Sep 25, 2024
@malvarezphrase malvarezphrase force-pushed the feature/STRINGS-374-enable-users-to-provide-a-prefix-during-cli-push branch from 2dacc6a to 3a6c754 Compare September 25, 2024 13:39
@malvarezphrase malvarezphrase changed the title feat: STRINGS-374 Enable users to provide a prefix during CLI push feat: STRINGS-374 Enable users to provide a prefix during CLI push and pull Sep 25, 2024
@malvarezphrase
Copy link
Contributor Author

@theSoenke & @jablan Let me know if I missed anything 🙏
And as far as I understood we release this once we untoggle the feature, right?

@malvarezphrase malvarezphrase changed the title feat: STRINGS-374 Enable users to provide a prefix during CLI push and pull feat: STRINGS-374 & STRINGS-439 Enable users to provide a prefix during CLI push and pull Sep 25, 2024
@jablan
Copy link
Collaborator

jablan commented Sep 25, 2024

hey @malvarezphrase can you please explain the logic with the truncating file name? I don't get it.

@malvarezphrase
Copy link
Contributor Author

hey @malvarezphrase can you please explain the logic with the truncating file name? I don't get it.

@jablan The reason for the truncate of the file name is that we want to use the idx_translation_keys_on_name_project_deleted index on translation keys when the prefix is used as a filter. Therefore we only allow it to be maximum 255 characters long.

Now looking at the other usages of the prefix, is it also possible to add a limit on create.yaml and download.yaml? Or will phrase respond with a validation error?

@jablan
Copy link
Collaborator

jablan commented Sep 25, 2024

@malvarezphrase sorry, I need a bit more context. where does <file_path> come from? is that a "magic" prefix working similar to <locale_code> and such? is it explained somewhere?

@malvarezphrase
Copy link
Contributor Author

@malvarezphrase sorry, I need a bit more context. where does <file_path> come from? is that a "magic" prefix working similar to <locale_code> and such? is it explained somewhere?

@jablan Yeah, it should be working like <locale_code>, so when <file_path> is given as the prefix, we want to use the actual file path as the prefix. But you're right, I'm missing the explanation. Where would I put it?

@jablan
Copy link
Collaborator

jablan commented Sep 25, 2024

Where would I put it?

@malvarezphrase I would start with the ticket description 😄 and probably the PR description too, as that stays longer. especially knowing that this repo is public.

if it can be specified only through the phrase.yml, probably we should document it in https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/phrase.json
but I don't see other magic placeholders either, so perhaps it's a good opportunity to add them all?

regarding the truncating, if I understand you correctly, we don't support prefixes longer than 255 chars? in that case we should probably error out rather than truncating them?

@malvarezphrase
Copy link
Contributor Author

@jablan Thanks a lot for the feedback! I already updated the ticket descriptions and also the description of this PR.

Are the changes to https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/phrase.json being done manually? If so, I will prepare a PR for it.

in that case we should probably error out rather than truncating them?
How do errors on the CLI usually work? The validation for the length is already in place inside phrase.

@jablan
Copy link
Collaborator

jablan commented Sep 26, 2024

perhaps then just passing whatever prefix is provided (either explicitly by the user or by using the filename) will be enough. is the error clear to the user?

as for json schema, perhaps you could open a small ticket, I see couple other things that could be fixed there, so we will do it all together.

just remember, we also have https://support.phrase.com/hc/en-us/articles/5808300599068-Using-the-CLI-Strings where we might want to describe this feature?

@malvarezphrase
Copy link
Contributor Author

@jablan & @theSoenke after changing to not truncate the length of the prefix and updating the description here, is there anything else missing?

doc/compiled.json Outdated Show resolved Hide resolved
@@ -190,6 +191,9 @@ func TargetsFromConfig(config phrase.Config) (Targets, error) {
if target.FileFormat == "" {
target.FileFormat = fileFormat
}
if target.Params.TranslationKeyPrefix.Value() == "<file_path>" {
target.Params.TranslationKeyPrefix = optional.NewString(target.File)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm not 100% sure but I think at this point target.File can still contain other placeholders, i.e. it won't be the path to the actual file. can you double check this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's important! Really good catch and it does still contain placeholders at this point

@theSoenke
Copy link
Collaborator

@jablan @malvarezphrase I've changed the file_path placeholder to make it work with other placeholders. It's also now possible to use the placeholder as part of the prefix like <file_path>-foo. Having the ability to separate the path from the key name by characters seems useful when I was testing it

@theSoenke theSoenke force-pushed the feature/STRINGS-374-enable-users-to-provide-a-prefix-during-cli-push branch from 376ce90 to f0a0ef4 Compare October 2, 2024 08:45
@theSoenke theSoenke requested a review from jablan October 2, 2024 09:40
Copy link
Collaborator

@jablan jablan left a comment

Choose a reason for hiding this comment

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

👌

@theSoenke theSoenke changed the title feat: STRINGS-374 & STRINGS-439 Enable users to provide a prefix during CLI push and pull feat: Add translation key prefixes for upload and download Oct 2, 2024
@theSoenke theSoenke merged commit 9c9c959 into master Oct 2, 2024
13 checks passed
@theSoenke theSoenke deleted the feature/STRINGS-374-enable-users-to-provide-a-prefix-during-cli-push branch October 2, 2024 12:17
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