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

BaseNode.equals support, and other ast goodness from python-fluent #172

Merged
merged 5 commits into from
Mar 25, 2019

Conversation

Pike
Copy link
Contributor

@Pike Pike commented Mar 19, 2018

This is basically a port of
https://github.com/projectfluent/python-fluent/blob/90301381e9b4d56b317bd6f15adf38936569f023/fluent/syntax/ast.py#L76-L120

Creating a PR early on to ensure that node 6 and 8 pass on the js features. For example, the use of Object.keys, which is rumored to be OK per http://node.green/#ES2015-misc-Object-static-methods-accept-primitives-Object-keys.

@Pike Pike changed the title BaseNode.equals support, wip, needs more tests BaseNode.equals support, and other ast goodness from python-fluent Feb 27, 2019
@Pike Pike requested a review from stasm February 27, 2019 13:07
@Pike
Copy link
Contributor Author

Pike commented Feb 27, 2019

stas, I think this is ready for review.

I gave all those things from python-fluent a go (w/out traverse).

Tests are heavily inspired by the python tests, too.

Questions I know of: ignoreFields vs ignore_fields. And is Object.create the right way to instantiate a new object of the same type, given that the kwargs trick in python doesn't work in js? (I don't think it does).

Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

Nice work! I'll finish the review tomorrow. For now, I'm leaving a few comments for your consideration.

fluent-syntax/src/ast.js Outdated Show resolved Hide resolved
fluent-syntax/src/ast.js Outdated Show resolved Hide resolved
fluent-syntax/src/ast.js Outdated Show resolved Hide resolved
fluent-syntax/src/ast.js Outdated Show resolved Hide resolved
fluent-syntax/src/visitor.js Outdated Show resolved Hide resolved
fluent-syntax/src/ast.js Outdated Show resolved Hide resolved
fluent-syntax/src/ast.js Outdated Show resolved Hide resolved
fluent-syntax/src/visitor.js Outdated Show resolved Hide resolved
assert.strictEqual(thisNode.value.equals(otherNode.value), true);
assert.strictEqual(thisNode.value.equals(otherNode.value, []), false);
assert.strictEqual(thisRes.equals(thisRes.clone(), []), true);
assert.notStrictEqual(thisRes, thisRes.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test which verifies that the original nodes have their variants in unchanged order after these tests run, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm dropping the ordering completely, per comment above, and update the tests accordingly. Let me know if there's more to do here.

This is different to python-fluent, as .equals doesn't ignore ordering
of Attributes and Variants anymore. Issue on python-fluent is filed.

Added an explicit test to ensure that SomeNode.clone
creates the same class.
@Pike
Copy link
Contributor Author

Pike commented Mar 13, 2019

There's still an open question I have on sortingKey. It's really useful in tooling as a canonical representation of a VariantKey, in compare-locales, and also in Pontoon for example.

@stasm
Copy link
Contributor

stasm commented Mar 14, 2019

Thanks for addressing my comments, @Pike! This looks good to land.

Regarding the question about sortingKey: serializedKey which we discussed last night would work for me. I think a helpful excerise is to imagine what this field would be for variants with StringLiterals as keys (projectfluent/fluent#90). Would we want to serialize them wrapped in double quotes, to differentiate them from keys which are Identifiers? Or should [foo] and ["foo"] serialize to the same thing?

I'm a bit concerned that having this field is mixing concerns. The use-cases present in Pontoon should really just use FluentSerializer or a custom serialization layer. The use-case of finding duplicate variants could also be solved with a one-line function in compare-locales. But, since this is an extra API of python-fluent's AST rather than part of the reference AST, I see the tooling value here and I don't want to block.

@Pike
Copy link
Contributor Author

Pike commented Mar 19, 2019

I've taking your comments and mulled over them, and then ended up removing the sorting_key in python. I replaced the use in compare-locales in https://phabricator.services.mozilla.com/D23813, which had a couple of arguments to change the way that code worked to begin with. For variant keys, I went for serialization. For attributes, I just hard-coded the AST structure accessor.

@stasm stasm merged commit bbcb340 into projectfluent:master Mar 25, 2019
@Pike Pike deleted the BaseNode.equals branch August 5, 2019 09:52
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