Skip to content

Remove variant lists #206

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

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Nov 6, 2018

Fix #204#214.

@stasm stasm force-pushed the remove-variant-lists branch from 7a2039f to ee1d8b5 Compare November 6, 2018 12:02
@stasm
Copy link
Contributor Author

stasm commented Nov 6, 2018

This PR depends on #205. I'll wait for it to land before I request review. @Pike, I'm just putting this on your radar for now, in case it makes reviewing #205 easier.

@stasm stasm force-pushed the remove-variant-lists branch from ee1d8b5 to d894a70 Compare November 7, 2018 11:20
@stasm stasm force-pushed the remove-variant-lists branch 2 times, most recently from 1c70ca8 to 4695aa7 Compare November 8, 2018 15:42
@stasm stasm requested a review from Pike November 8, 2018 15:42
Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

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

There's technical changes I'd like to see here.

I think it'd be good to have tests that cover the grounds we're leaving behind, and declare them as parsing errors. With that, we at least prevent accidental syntax re-use, and keep a reminder that we've tried this and didn't like at least some of it.

There's also a ton of conflicts and there'll probably be more by the time 0.8 is out, but we'll get there when it's time.

@@ -1,6 +1,5 @@
variant-expression = {-term[case]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this as an error test?

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 added a new fixture file, called obsolete.ftl, with a few examples of discontinued syntax. Thanks for the suggestion.

-simple-identifier =
{
simple-identifier =
{ $sel ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you cross-check which of these are already tested in select_expressions.ftl ?

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 checked. select_expressions.ftl is mostly about selectors and things like empty variant values. In variant_keys.ftl I'd like to test different variant keys and their errors. I made a few changes in select_expressions.ftl to simplify the variant keys used there (key or one and two for nested placeables).

@stasm stasm force-pushed the remove-variant-lists branch from 4695aa7 to ab9bd58 Compare January 9, 2019 18:03
@stasm stasm requested a review from Pike January 9, 2019 18:05
@stasm stasm merged commit deb1dd5 into projectfluent:master Jan 10, 2019
@stasm stasm deleted the remove-variant-lists branch January 10, 2019 10:52
@stasm stasm mentioned this pull request Jan 10, 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

Successfully merging this pull request may close these issues.

2 participants