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

sql/ir: use a special character to highlight metavars in templates #18398

Closed
wants to merge 1 commit into from

Conversation

knz
Copy link
Contributor

@knz knz commented Sep 9, 2017

No description provided.

@knz knz requested review from justinj and a team September 9, 2017 19:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20170909-middle-dot branch from 8d77ebc to b2a7574 Compare September 9, 2017 19:15
@petermattis
Copy link
Collaborator

The use of the special character is interesting, though I'm not sure if the need is compelling enough to justify the usage. A unique prefix would also suffice.


Review status: 0 of 5 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Sep 13, 2017

Do you have a better suggestion?

@petermattis
Copy link
Collaborator

Does the template code need to be valid go code? If not, I'd use something like <numRefsPerNode>. If the template code does need to be valid go code, perhaps something like META_numRefsPerNode.


Review status: 0 of 5 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@justinj
Copy link
Contributor

justinj commented Sep 22, 2017

I can see an argument against having "normal" characters given that metavariables could occur anywhere, including in strings. Does seem like a bit of a pain to work with though... I don't have a suggestion for something that would solve both problems short of moving to a homoiconic language :) so I'll give a weak +1 to using the unicode character, personally.

@knz
Copy link
Contributor Author

knz commented Jan 25, 2018

Superseded by #19135.

@knz knz closed this Jan 25, 2018
@knz knz deleted the 20170909-middle-dot branch April 27, 2018 18:38
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.

4 participants