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

Add support for web extension messages.json & plain key-value JSON files #2

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

eemeli
Copy link
Member

@eemeli eemeli commented Apr 11, 2024

The parser and serialiser are pretty strict for this, as the messages.json format is pretty explicit. It also effectively requires using the MF2 data model representation, does not allow for metadata, and is also pretty strict on the keys it supports.

@eemeli eemeli requested a review from mathjazz April 11, 2024 16:41
@eemeli eemeli changed the title Add support for web extension messages.json files Add support for web extension messages.json & plain key-value JSON files Apr 11, 2024
@eemeli
Copy link
Member Author

eemeli commented Apr 11, 2024

Also added nested key-value JSON files.

Copy link
Contributor

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

I'd change the paths, other than that is looks good.

from .parse import plain_parse
from .serialize import plain_serialize

__all__ = ["plain_parse", "plain_serialize"]
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the folder doesn't reflect the name of the format (json) as is the case with other formats.

I expect that to result in confusion when searching for the relevant code.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was an intentional choice, to account for the possibility of this structure working with other resource formats, such as YAML.

from .parse import webext_parse
from .serialize import webext_serialize

__all__ = ["webext_parse", "webext_serialize"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as with the plain JSON support - I would expect the name to include the word json.

"placeholders",
"1your_name",
"content",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I didn't even realize Pontoon already supports multiple nesting levels in key-value JSON files. Just checked and... it does.

["placeholders"],
PatternMessage(
[
"Hello$$ ",
Copy link
Contributor

Choose a reason for hiding this comment

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

It puzzles me why is this not "Hello$$$ ".

Copy link
Member Author

@eemeli eemeli Apr 15, 2024

Choose a reason for hiding this comment

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

From here:

Any number of consecutive dollar signs appearing in strings are replaced by the same number of dollar signs minus one. So, $$ > $, $$$ > $$, etc.

@eemeli
Copy link
Member Author

eemeli commented Apr 15, 2024

I'm going to keep the paths for now, and iterate on them later as the whole shape of this comes into clearer focus.

We could e.g. end up with moz.l10n.resource.webext, and going that deep adding a _json there starts to feel like one more hierarchy layer, even if it's just in the name.

@eemeli eemeli merged commit cc48921 into main Apr 15, 2024
7 checks passed
@eemeli eemeli deleted the webext branch April 15, 2024 10:41
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.

2 participants