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 Terms #205

Merged
merged 2 commits into from
Nov 8, 2018
Merged

Add Parameterized Terms #205

merged 2 commits into from
Nov 8, 2018

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Nov 6, 2018

Fix #176.

@stasm stasm mentioned this pull request Nov 6, 2018
@stasm stasm force-pushed the macros branch 5 times, most recently from e2f341a to d6a3a25 Compare November 6, 2018 12:09
@stasm stasm changed the title WIP Add Parameterized Terms Add Parameterized Terms Nov 6, 2018
@stasm stasm requested a review from Pike November 6, 2018 12:11
@stasm stasm mentioned this pull request Nov 6, 2018
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.

Can you please open a separate issue for the changes to function names?

I'm wondering if we could formulate the parametrized terms better. How about replacing TermReference in InlineExpression with TermExpression, and maybe factor the argument list a bit to include the full call expression postfix?

I picture this from a resolver implementation, and I expect the implementation to set up the environment for Terms to be rather different than to that of a call into the globals. So implementers might do the above anyway, but they'd need to piece that together?

This would also avoid the addition to valid.md? There'd be renames there, though.

@stasm
Copy link
Contributor Author

stasm commented Nov 7, 2018

Can you please open a separate issue for the changes to function names?

Sure: #209.

I'm wondering if we could formulate the parametrized terms better. How about replacing TermReference in InlineExpression with TermExpression, and maybe factor the argument list a bit to include the full call expression postfix?

That's an interesting approach. However, I see CallExpression as being useful for macros because of its generality. We might want to enable the -term.attr() syntax soon (#189). And later, dynamic references will likely require $var() (#80).

I picture this from a resolver implementation, and I expect the implementation to set up the environment for Terms to be rather different than to that of a call into the globals. So implementers might do the above anyway, but they'd need to piece that together?

Or they can check the type of the callee.

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.

I'm not happy with the split of where term references are after this patch. But I also see that we might have { $animal(decl: "nominative") } at some point, where the point that we're have a TermReference as callee won't be visible to the parser.

I filed #213 to document what we're doing here.

key01 = { -term }
key02 = { -term() }
key03 = { -term(arg: 1) }
key04 = { -term("positional", narg1: 1, narg2: 2) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are positional arguments intended to be legal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're not used by the runtime to resolve the term so even if they're passed, they're just ignored. We could forbid them in abstract for now, in case we find a use for them in the future. How does that sound to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered about abstact, too, but no idea if there's going to be a use or not. I'm not a friend of "for now", either, so, well. Yeah, I filed a documentation bug. No strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave them as valid, then. Thanks for filing the issue about the docs.

@stasm stasm merged commit dc20d52 into projectfluent:master Nov 8, 2018
@stasm stasm deleted the macros branch November 8, 2018 15:22
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