-
Notifications
You must be signed in to change notification settings - Fork 11
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
Pain points with the examples and API, plus recommendations #1
Comments
Thanks! I agree with all your comments.
Please expand on this point. What changes do I need to make? |
Instead of function load_parser(option = {}) {/*...*/}
//...
module.exports = {
//...,
load_parser,
}; you do export function load_parser(options = {}) {/*...*/} The optional secondary Rollup pass is a bit involved, as it requires installing npm modules. This could be deferred to the user. |
I see. Is there a short-hand to export all the symbols at once? |
Yes, you can also do export {
//...
load_parser,
}; |
I think these are some great comments. I would in particular like to second the request for ESM support, which I think is as simple as updating the export syntax to ESM (that said, I've no idea if other Lark.js consumers expect CJS output, so I don't know how disruptive this change would be). I don't think it's necessary to provide different flavors of the output file (CJS / UMD) as most of the ecosystem supports ESM natively, and for those that don't transforming is commonly an end-user requirement. Two other pain points I'd throw out there:
|
First, some positive notes.
General pain points and recommendations:
PYTHONPATH
environment variable in order to be able to executelark-js
is inconvenient.lark
. This may give the impression to users that they get a full JS equivalent of the original Lark library. I could not tell immediately from the code whether this is actually the case, but if it isn't, I recommend going with a different name that is more specific to the parsed grammar. For example,json_parser = require('./json_parser.js');
.load_parser
is a slightly confusing name for the main interface entry point, because the word "load" could also mean that you're supposed to pass the parser itself as an argument. I suggestget_parser
or justparser
instead.transformer
was optional. Leaving it out was instructive, but I would not have been able to predict the difference without trying. I recommend expanding the top comment to the point that it alone is sufficient to make basic use of the mentioned options. Options that would require too much explanation can be left out and deferred to the standalone documentation.To test: postlex, lexer_callbacks, g_regex_flags
is probably just a "note to self", but I must point out that it is extremely cryptic to an outsider reading the code. If it is a note to self, I suggest moving it to an issue ticket. If it is meant as a suggestion for the user, you need to expand considerably on how to use these parameters.lark-js
currently generates a CommonJS module, but the generated code is portable enough to also work in other environments than Node.js. In addition to that, a growing number of Node.js users is expecting the newer ES6 module (ESM) format. Tools like Rollup can easily convert ESM to other module formats, such as pure CommonJS for Node.js and UMD (a hybrid of CommonJS, AMD and browser global) for browsers. For these reasons, I strongly recommend generating the output as ESM instead. You can optionally generate UMD and pure CommonJS formats as well, by doing a secondary Rollup pass over the ESM version as an added service to the user.Pain points and recommendations specific to the example JSON parser:
run_json_parser
script. This results in a double escape notation, which is potentially confusing for new users (among other things, they might wrongly conclude that the second backslash is the character being escaped or that Lark always requires all escapes to be doubly escaped). I recommend moving the example JSON into a separate file so that users see the text as Lark sees it "au naturel".Pain points and recommendations specific to the example Python 3 parser:
BASE_DIR
constant is hardcoded to a Windows-specific directory. I strongly recommend changing this to a project-relative directory. If incorporating Lark.js in Lark proper, you can pass over all Python modules in Lark, which should be more than enough to demonstrate the parser's capabilities. If keeping Lark.js separate, you can use the lark-js directory. While it contains only one small Python file, this has the advantage that you can print the parse tree without flooding the terminal.node run_python_parser.js
, with theBASE_DIR
changed to../lark-js/
and an added line to print the parse tree to the console, resulted in a SyntaxError, see below. My obvious suggestion is to fix it.The text was updated successfully, but these errors were encountered: