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

Allow specifying the priority for PO import/exports in the flags. #1341

Merged
merged 9 commits into from
Mar 23, 2022

Conversation

dd32
Copy link
Contributor

@dd32 dd32 commented Mar 22, 2022

Fixes #1340

This allows for the .po file to contain a high-priority flag, removing the need for external code to update the database afterwards.

Note: This does not reset the priority to normal unless normal-priority is explicitly added.

The flags looked at are therefor: low-priority, normal-priority, high-priority, and hidden-priority

I went with the format of priority=TEXT due to the way I started down this rabbit hole.

Potentially this could be changed to just being low-priority, normal-priority, high-priority, hidden-priority instead, actual flags rather than a key-value pair. I'm creating this for code-review on the approach.

No tests included, but it does work (at least on WordPress.org)

@dd32 dd32 added the [Type] Enhancement A suggestion for improvement. label Mar 22, 2022
@dd32 dd32 force-pushed the add/priority-import-export branch 2 times, most recently from 803cde2 to c017b82 Compare March 22, 2022 08:39
@ocean90 ocean90 added this to the 3.0 milestone Mar 22, 2022
@ocean90
Copy link
Member

ocean90 commented Mar 22, 2022

Clever idea using flags! Looking at the docs of gettext and it doesn't seem like there's a specific syntax defined but most of them are in kebab-case with the exception of the range flag. I guess priority: low would be an option too. But here I think we should just go with low-priority.

Having some tests would be nice and shouldn't be that hard to write. Here's the example for fuzzy: https://github.com/GlotPress/GlotPress-WP/blob/fae8e0035385f1efd7248db604aed692b46f70ba/tests/phpunit/testcases/tests_things/test_thing_translation_set.php#L33-L47

@dd32 dd32 force-pushed the add/priority-import-export branch from 3c19e00 to 5f02ecd Compare March 22, 2022 10:05
@dd32
Copy link
Contributor Author

dd32 commented Mar 22, 2022

I guess priority: low would be an option too. But here I think we should just go with low-priority.

While priority: hidden/priority: normal would read better than hidden-priority/normal-priority I expect the readability of low-priority/high-priority is more worthwhile here, so might as well just go with that.

I had read a few gettext pages for the flags field, but I didn't spot anything that suggested a valid set of characters, only that it split by ,. I got lost trying to find it in the sourcecode.

PR updated, and a test included, even if I did write it without running it locally.. with a PHP Syntax error originally.. :)

@ocean90
Copy link
Member

ocean90 commented Mar 22, 2022

While priority: hidden/priority: normal would read better than hidden-priority/normal-priority

Argh, naming is hard… Since it seems using a colon is fine maybe go with that version?
I'm now even wondering if we should prefix the custom flag with gp-? As in gp-priority: hidden, gp-priority: normal, gp-priority: low, and gp-priority: high.

@dd32
Copy link
Contributor Author

dd32 commented Mar 23, 2022

Argh, naming is hard…

Tell me about it...

I'm now even wondering if we should prefix the custom flag with gp-

I too had thought about that... I ultimately went with not-prefixed so that it would be a more natural flag in exported files in other tools, Seeing low-priority there would be more relevant than gp-.....

But I also don't know how 3rd party tools will process flags, or if they even will display it? seems like it's more likely that other tools simply won't do anything when it see's these flags, and if it'll never be displayed, it doesn't matter if it's readable or not.. just concise/easy to parse/accurate.

gp-priority: hidden, gp-priority: low, gp-priority: normal, gp-priority: high works for me though, and is just as good as the other options, if something displays it, or parses it, it should be obvious that a) it's a custom GlotPress thing and b) it's a priority field and a value.

I had also thought about cramming the priority into the Comment section for that purpose, but ultimately decided that using the "internal" translation flags seemed more appropriate than a human-readable area..
It would've been compatible with other translation formats going that route, but it might just be that .po support is the only format that currently implements flags.

@ocean90 ocean90 force-pushed the add/priority-import-export branch from f868e94 to 7514e02 Compare March 23, 2022 08:13
@ocean90 ocean90 enabled auto-merge (squash) March 23, 2022 08:14
@ocean90 ocean90 merged commit 76ee201 into GlotPress:develop Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to specify the priority of a string during import
2 participants