Skip to content

Create a FTL serializer #184

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

Closed

Conversation

Michael-F-Bryan
Copy link
Contributor

Fixes #182.

@Michael-F-Bryan Michael-F-Bryan marked this pull request as ready for review June 23, 2020 09:38
@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Jun 23, 2020

I've added all tests down to "nested select expression" and they all pass locally. At this point there are no more unimplemented!()s so I believe I'm hitting every branch, now I just need to make sure this is identical to the TypeScript implementation.

@zbraniecki, would you be able to nominate someone as a reviewer? I'm going to finish up for today, but this should reach parity with the TypeScript serializer by tomorrow morning.

@zbraniecki zbraniecki self-requested a review June 23, 2020 17:34
@zbraniecki
Copy link
Collaborator

@zbraniecki, would you be able to nominate someone as a reviewer? I'm going to finish up for today, but this should reach parity with the TypeScript serializer by tomorrow morning.

Looks good! I'll try to review by EOW.

@Michael-F-Bryan
Copy link
Contributor Author

Thanks @zbraniecki! We'll use git dependencies for now, so that takes the pressure off you.

I've implemented all the TypeScript tests and they pass, so this should be identical to the TypeScript implementation. Having an existing serializer I can copy from, complete with a thorough test suite, made this job really easy 🙂

@Pike
Copy link
Contributor

Pike commented Aug 7, 2020

Watch out for projectfluent/fluent.js#512

@Michael-F-Bryan
Copy link
Contributor Author

@zbraniecki, any chance you can review this?

@zbraniecki
Copy link
Collaborator

@zbraniecki, any chance you can review this?

I will. I'm sorry for the delay. As you may have heard, we're going through a bit of a change and I'm per-occupied with some non-technical tasks, but fluent-rs is maintained and I do intend to get another release soonish.

Please, be patient and feel free to use a fork in the meantime, it shouldn't bitrot.

@mainrs
Copy link

mainrs commented Sep 3, 2020

On a side note, wouldn't it be more beneficial to use serde and leverage its powers? You don't have to use the derive macros that serde provides but can implement the traits on your own. The real benefit comes from being able to serialize to other file formats than text.

In such a case, an application could theoretically provide its l10n files in a binary format through Over-the-air updates, reducing the number of bytes to transfer compared to using a text representation. Deserializing using serde would allow users to also use binary formats, reducing the overall file size. It's probably not much, but on smaller systems like embedded devices, a few bytes are already worth it.

Edit: I do not mean to critique the PR (which looks good btw!). It's just an idea that should probably be considered for the future as it allows to expand the ecosystem.

@Michael-F-Bryan
Copy link
Contributor Author

@sirwindfield have you seen the fluent_syntax::json module?

The main reason to also have a manual serializer is because the serde serializer API isn't really compatible with a pretty-printer.

serde works by encoding a state machine that can traverse a JSON-esque tree of data. Some big assumptions are that how something is serialized won't change based on its value or the value of a child-node and that you're just dealing with atoms (strings, numbers, etc.), sequences (lists), and maps (maps and structs).

On the other hand, how you print parts of a fluent_syntax AST depends very much upon their neighbours or their child nodes. You may need to insert whitespace and special-case some nodes to make sure the final result looks correct to a human.

In such a case, an application could theoretically provide its l10n files in a binary format through Over-the-air updates, reducing the number of bytes to transfer compared to using a text representation.

I'd just compress the files. The vast majority of the AST is plaintext that wouldn't disappear after being parsed, so you don't gain much by using a binary format.

@zbraniecki
Copy link
Collaborator

I think this would be great now that we have AST over S. You could make it work for any Slice<ToString>, and this way any AST will be serializable!

I'm planning to generalize Resolver soon to work on any Slice<AsRef<str>>.

@zbraniecki
Copy link
Collaborator

Hi @Michael-F-Bryan . I'm going to close this PR in favor of a follow up in #241.
If you think we should keep it open, let me know

@gregtatum gregtatum closed this Oct 27, 2022
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.

FTL Serializer
5 participants