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

Distinguish 'single'- and "double"-quoted tokens in CREATE GENERATOR. #410

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

riastradh-probcomp
Copy link
Contributor

No existing users should be affected unless they inadvertently used
single-quotes within a Crosscat schema -- which is something we ought
to detect and reject early anyway.

First small step in preparation for future changes to pass on the
token stream verbatim to the metamodel's schema parser, rather than
munging it into nested lists & strings for convenience.

No existing users should be affected unless they inadvertently used
single-quotes within a Crosscat schema -- which is something we ought
to detect and reject early anyway.

First small step in preparation for future changes to pass on the
token stream verbatim to the metamodel's schema parser, rather than
munging it into nested lists & strings for convenience.
@gregory-marton
Copy link
Contributor

With admittedly almost no info on what the future changes are that this is a small step for, but given that comparison, I find myself starting not in favor, because this section of code is slated to get killed under the bql/mml split design[1]. To the extent that even this change is risky, (the "should" above frightens me a little) I'd rather not adopt it unless @axch is depending on it happening. Please advise?

[1] https://docs.google.com/document/d/1lHNVlxfpjLTUGYZHWDZkmPsOwLNGdYMupAUdfw32eaM/edit#

@riastradh-probcomp
Copy link
Contributor Author

I put the 'should' there to do the opposite of frighten you -- to explain why this change is not particularly risky, which I assumed would be unnecessary to explain under the premise of light review but which I felt a personal responsibility to include anyway. Should I have added the word 'unlikely' to that sentence? (I thought the unlikeliness of the mistake would be obvious to anyone who has ever written any Crosscat schemas -- there is nowhere even remotely tempting to write single-quoted strings.)

As far as I can tell the BQL/MML split still has places where exactly the same kind of token sequence passage will occur -- whatever parser eats up CREATE METAMODEL foo; INITIALIZE ...; will want to pass the token sequence in ... verbatim to some metamodel method just like we do now. Maybe it won't be in the same bayeslite/src/grammar.y file but I expect it will be convenient to have exactly the same mechanism already articulated and tested.

I'm not sure whether @axch depends on this now but it would make certain syntax easier to implement safely in the elaborated gpmcc language.

@gregory-marton
Copy link
Contributor

Re light review, I'm not sure that the stated purpose here advances the project's goals. Willing to be convinced, but I'd rather postpone the question unless @axch is indeed planning to rely on this in the short term.

@axch
Copy link
Contributor

axch commented May 13, 2016

For the record, I do not plan to rely on this in the short term. Then again, I also don't see any reason not to merge it if it passes tests; it's one of the kinds of things that if someone other than @riastradh-probcomp came across later, they would fear it substantially more.

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.

3 participants