kach / nearley

📜🔜🌲 Simple, fast, powerful parser toolkit for JavaScript.
https://nearley.js.org
MIT License
3.59k stars 232 forks source link

Allows javascript keywords as literals #567

Closed AsLogd closed 3 years ago

AsLogd commented 3 years ago

Looking at the generated typescript type for the tokens I guessed the real intent, so please correct me if I guessed wrong. In my understanding there were missing quotes when building tokens for the parser rules. Fixing this caused the typescript tests to fail, because the (incorrectly) used variables where given any type, thus making the ternary expression adopt this broader type and passing the test. Then I fixed the type to what I think it should be.

fixes #525

kach commented 3 years ago

Hmm… this seems reasonable. But perhaps you want to use JSON.stringify instead of "\""+…+"\"" concatenation to quote the string, as the rest of that line of code does?

(cc @tjvr)

AsLogd commented 3 years ago

We could use template literals. Support seems to go as far as 4.9. The line would look like this:

var expr = `${result.config.lexer}.has("${name}") ? {type: "${name}"} : "${name}"`;

I could also change other occurrences of JSON.stringify if you like that. Else, I would just use stringify to match the current style. It's your call.

EDIT: This would work only if name is always a string. I'm not sure if name can be expected to be an object or other values (but doesn't look like it)

AsLogd commented 3 years ago

Hi, is there something I could do to speed things up? This fix could be pretty convenient for me right now,

kach commented 3 years ago

Thanks for the reminder, and sorry for the delay. (It's hard maintaining a package for free as a full-time student…)

Can you change it to indeed use JSON.stringify as discussed above?

AsLogd commented 3 years ago

No problem. I'm truly grateful for your work and I'm impressed that you could develop it while studying, I'm not super familiar with nearley, but feel free to tag me on other issues/pull requests if you need another set of eyes.

I made the requested change, Just to confirm, the 'nuller' test is not passing on master, right? (is not passing on this branch either)

kach commented 3 years ago

The nuller test is skipped — it isn't run at all.

AsLogd commented 3 years ago

Is this ready to be merged then?

kach commented 3 years ago

Ah, so I took another look the other and actually I think the current behavior is correct and indeed what is intended: if the lexer doesn't has() a token with that name, then the next fallback is to check if there is a variable with that name in scope that describes the token. I think this was a backwards-compatibility-related thing.

The right move is probably to emit a warning (in the linter) if you try to use a keyword as the name of a lexer token… someday I'll get around to doing that… :)

AsLogd commented 3 years ago

I see. All right. Thanks

rizqventure commented 3 years ago

_**```