search

projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.05k stars 95 forks source link

Create a FTL serializer #184

Closed Michael-F-Bryan closed 1 year ago

Michael-F-Bryan commented 4 years ago

Fixes #182.

Michael-F-Bryan commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 3 years ago

Watch out for https://github.com/projectfluent/fluent.js/issues/512

Michael-F-Bryan commented 3 years ago

@zbraniecki, any chance you can review this?

zbraniecki commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 2 years ago

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