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

Return callback values in parser to allow synchronous parsing #1499

Merged
merged 1 commit into from
Sep 14, 2013

Conversation

brenmar
Copy link
Contributor

@brenmar brenmar commented Aug 20, 2013

This is a simple very minimal impact fix for #1498, allowing the parser to be used synchronously when imports do not need to be processed. It also aligns with the convention used in the parser for handling errors, when errors are handled the result of the callback is returned in the current implementation. This fix aligns with that exisitng convention.

This fix allows synchronous execution of the parser using the following code:

var parser = new(less.Parser)({
    processImports: false
});
var result = parser.parse(lessSource, function(e, r) {return {error: e, success: r};});

@lukeapage
Copy link
Member

Its a minimal change, but if we take this, we have to support and bugfix it, where as you could just wrap the function (as in the example in the previous bug) and it would work forever.. so I'm not sure about this, even though it is a tiny non breaking change.

@brenmar
Copy link
Contributor Author

brenmar commented Aug 21, 2013

Well for consistency within the function the results of the callback should be returned, as the other 2 places it is already (where it is a synchronous code path) in the current code, see lines 359 and 371, shown in the snippet below:

if (error) {
    return callback(new(LessError)(error, env));
}

// Start with the primary rule.
// The whole syntax tree is held under a Ruleset node,
// with the `root` property set to true, so no `{}` are
// output. The callback is called when the input is parsed.
try {
    root = new(tree.Ruleset)([], $(this.parsers.primary));
    root.root = true;
    root.firstRoot = true;
} catch (e) {
    return callback(new(LessError)(e, env));
}

Consistency of code will always go towards reducing support and big fix requirements. It also looks like an oversight of the addition of the processImports false change as this added a new synchronous path to the code (in addition to the 2 existing paths that returned their value shown above), but the convention that was established with those was not followed by that change, this fix re-establishes a standard convention and ensures that all synchronous paths return data. Therefore any future modifications can clearly see what should be done in the case of a synchronous path, because at present there are 2 conventions used, and as any professional knows that is more likely to lead to future inconsistencies, support issues and bug fixes.

@lukeapage lukeapage merged commit 1ca52ff into less:master Sep 14, 2013
@lukeapage
Copy link
Member

okay, pulled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants