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 AST and en/pl examples #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add AST and en/pl examples #7

wants to merge 2 commits into from

Conversation

zbraniecki
Copy link
Owner

@zbraniecki zbraniecki commented Jul 9, 2020

Here's an initial proposal based on Option 2 from #6, using Placeholder from #4.

@zbraniecki zbraniecki requested a review from echeran July 9, 2020 17:23
@zbraniecki
Copy link
Owner Author

zbraniecki commented Jul 9, 2020

@mihnita @stasm - how does it look to you?

@mihnita
Copy link

mihnita commented Jul 10, 2020

I think it implements (almost) the use case we wanted to implement.
It does not show the use of a numeric placeholder, but shows more in other areas (plural, select).

I find the examples really hard to read (just because there is a lot of "duck tape" building the objects, not because it is rust syntax). See below.

I find it a lot easier to just read the ast.rs
Which is where we want to focus anyway :-)

I think it "propagates" some of the existing problems
(like the lack of support for combinations of selectors, what is now nesting in the old MF and in Fluent)
But we don't want to do a "one big commit that resolves everything" anyway :-)

In general I wanted to stay away from your experiment here, so that what I came up with is independent
(if we independently come up with similar structures that is a good sign :-)
And we might be able to "merge" things in ways that would solve more of the requests.

So I apologize if I didn't get involved (until I've seen you pinging me directly :-)


I find the building of the message really hard to read.
Not a complaint, I have the same problems with my prototyping using protobuffers.
My json prototyping is a bit more readable than the proto one.
But still not readable enough.

The ast is pretty readable though.

I wonder if using serde to dump the message in a different syntax (json, yaml?) would be more readable.
Or maybe even some "macros" that generates the real ast?

For example:

PatternElement::Placeholder(InlineExpression::VariableReference(Identifier(
            "userName".into(),
        ))),

replaced by:

ph("userName")

The whole English message might be:

    let _en = Message_Single([
        ph("userName"),
        text(" published a post in "),
        ph("groupName"),
    ]);

I am not sure what exactly can be done with the rust macros.

use message_format::ast::*;

pub fn main() {
// Anne published a post in Birthday Party
Copy link

Choose a reason for hiding this comment

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

Suggestion: add the generic form, as represented by the code:
{userName} published a post in {groupName}
(or, if we don't want it ICU, {$userName} published a post in {$groupName})

Copy link
Collaborator

@echeran echeran left a comment

Choose a reason for hiding this comment

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

LGTM.

I'm similar to Mihai in that I'm having to go off and experiment on my own to see if my ideas actually work as intended, and to learn more Rust in the process (including getting the compiler to do what I want). My work is in the fork of this repo, very much a WIP, I can already see some similarities and differences. But I think it'll help my ability to have an informed opinion to continue exploring & learning.

I think this covers all [https://github.com/unicode-org/message-format-wg/pull/94/files?short_path=50e1d2b#diff-50e1d2bd4aa7bab895a42ce05dfcc5c4](the examples) up to and including gender+plural. I think there was discussion in a diff thread on what would happen if both gender and plurals were needed as selectors in the same mutli-message group, and if not, that would be interesting to explore.

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