Skip to content

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