-
Notifications
You must be signed in to change notification settings - Fork 65
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
Better handling of reserved words + ability to redefine them + Plugins API doc #150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of these are minor nits, but the ones about the passes structure need more attention than the others.
docs/documentation.html
Outdated
<link rel="stylesheet" href="/css/common.css"> | ||
<link rel="stylesheet" href="/css/layout-default.css"> | ||
<link rel="stylesheet" href="/css/content.css"> | ||
<link rel="stylesheet" href="./css/common.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with this, but let's make the change in the rest of the html files in docs
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that shouldn't be here -- I was just testing how the documentation looks and accidently commit that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use light-server
to serve the docs locally, but it would be nice for them to work with a file:
URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted that in this PR. It was committed accidently and I have no ideas how it will work on the real site.
@@ -38,13 +105,17 @@ const peg = { | |||
const plugins = "plugins" in options ? options.plugins : []; | |||
const config = { | |||
parser: peg.parser, | |||
passes: convertPasses(peg.compiler.passes) | |||
passes: convertPasses(peg.compiler.passes), | |||
reservedWords: peg.RESERVED_WORDS.slice(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: see peggyjs/peggyjs-eslint-config#2
src/parser.pegjs
Outdated
@@ -39,6 +39,9 @@ | |||
"!": "semantic_not" | |||
}; | |||
}} | |||
{ | |||
var reservedWords = new Set(options.reservedWords); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure to test the output of Typescript in old browsers to ensure this gets polyfilled adequately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to MDN, and our compatibility list we are losing Internet Explorer. Neither TypeScript, nor rollup, nor terser does not polyfill Set
, so we are not ready to use Set
here. I've even checked TypeScript in the ES3 mode. Nothing.
So I change it to the ordinary array. That should not be a big problem, because linear search in the list won't be much worse because that list hardly ever will big enough to see Set
benefits
- `config` — object with the following properties: | ||
- `parser` — `Parser` object, by default the `peg.parser` instance. That object | ||
will be used to parse the grammar. Plugin can replace this object | ||
- `passes` — mapping `{ [stage: string]: Pass[] }` that represents compilation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this points out that compiler.compile and the code that uses it in peg.js is overly-confusing and relies on Object.keys
to be consistent in the face of plugins making changes to the passes.
Let's look at that code as we document this interface, and see if it needs work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, according to the MDN, all works because of implicit assumption that keys always will be returned in the insertion order (which is equal to the declaration order for object literals). But I thin that this is implementation detail, conceptually my description should remains unchanged even if we change implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a plugin author, I want to add a pass that is between check
and transform
. I guess I could do it without breaking other plugins that did the same thing by creating a new object, traversing Object.keys(passes)
, inserting my pass after check
, and re-assigning passes. I guess that's close enough.
will be used to parse the grammar. Plugin can replace this object | ||
- `passes` — mapping `{ [stage: string]: Pass[] }` that represents compilation | ||
stages that would applied to the AST, returned by the `parser` object. That | ||
mapping will contain at least the following keys: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"at least" implies that plugins can add passes, which I think they can, but it will be hard for them to add them in the middle at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you right. I use that turn of speech to show that in the future list of stages could be expanded. For example, in my implementation of template rules (pegjs/pegjs#45) I've needed a precheck
stage. I'm not sure, that this was right choice, although. If someday I returns to that problem I review that choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor grammar nits, rebase, and I'll merge.
…dd documentation for plugins API Before this commit error looks like (for input `start = break:'a'`) > Expected "!", "$", "&", "(", "*", "+", ".", "/", "/*", "//", ";", "?", character class, code block, comment, end of line, identifier, literal, or whitespace but ":" found. After this error looks like > Expected identifier but reserved word "break" found.
Done. |
There is port of my PR pegjs/pegjs#552 with additions: