Skip to content

Consider renaming StringLiteral::raw to StringLiteral::value and StringLiteral::value to StringLiteral::decoded #222

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
zbraniecki opened this issue Nov 29, 2018 · 2 comments

Comments

@zbraniecki
Copy link
Collaborator

zbraniecki commented Nov 29, 2018

In line with my feedback from projectfluent/fluent.js#310, I'd like to suggest that we rename the main field of the StringLiteral to value and keep the optimization we have as value today with a new name like encoded which we could then mark as "optional" for implementations to introduce.

This would allow us to maybe remove the encoded field from test fixtures since it's an optimization for the fluent.js, rather than part of the reference AST. (or mark that field as optional for tests to pass).

@stasm stasm transferred this issue from projectfluent/fluent.js Nov 30, 2018
@stasm
Copy link
Contributor

stasm commented Nov 30, 2018

(I moved this issue to the spec repo since it affects the reference parser.)

In line with my feedback from projectfluent/fluent.js#310, I'd like to suggest that we rename the main field of the StringLiteral to value and keep the optimization we have as value today with a new name like encoded which we could then mark as "optional" for implementations to introduce.

The current design, including naming, was based on #195 (comment). The goal of adding the unescaped value to the AST was to standardize how escape sequences are intended to work. Before, the expected behavior wasn't specified at all.

The escape sequences can be unescaped statically during parsing, which makes the AST more usable. For tooling, the raw field is available, too. I see both fields as essential parts of the reference parser.

This would allow us to maybe remove the encoded field from test fixtures since it's an optimization for the fluent.js, rather than part of the reference AST. (or mark that field as optional for tests to pass).

I see a difference between the reference parser and the reference AST. As I think I mentioned before, I think the implementations should be free to use any AST they are most comfortable with, and are only required to be able to serialize it into a reference-compliant JSON structure.

@stasm
Copy link
Contributor

stasm commented Mar 5, 2019

This was fixed by #243.

@stasm stasm closed this as completed Mar 5, 2019
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

No branches or pull requests

2 participants