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 parameterized Attributes #212

Merged
merged 3 commits into from
Nov 14, 2018
Merged

Add parameterized Attributes #212

merged 3 commits into from
Nov 14, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Nov 8, 2018

Fix #189. Add syntax to allow expressions of the form -term.attr(arg: "value"), e.g.:

-ship.gender(style: "chicago")
-echo.last-letter(count: 2)

@stasm stasm force-pushed the attr-params branch 2 times, most recently from 881d8fc to 0bbd3dd Compare November 14, 2018 12:05
@stasm stasm requested a review from Pike November 14, 2018 12:09
@stasm
Copy link
Contributor Author

stasm commented Nov 14, 2018

This depends on #217 which is the first commit here. The second commit has the changes to the code, and the third one—tests. We were missing some coverage when it comes to validating expressions in different contexts.

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.

r=me, this looks much better, thanks.

I think there's a left-over validation for AttributeExpression? Doesn't hurt, but I'd remove it.

Also, as a follow-up, do you want to update valid.md to the changes here, too?

Neither blocking.

return always(new Type(ref, name));
}
return never(`Invalid ref type: ${ref.type}.`);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part is not needed with the reduced changes after the rebase? Attributes are always on messages or terms still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but I'd like to keep this here for the sake of completeness. I think it complements the validation of CallExpression. Let's keep it for now and I'll see if I can remove it in a follow-up.

@stasm stasm merged commit d011832 into projectfluent:master Nov 14, 2018
@stasm stasm deleted the attr-params branch November 14, 2018 16:00
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