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

Adding assign tag #190

Merged
merged 1 commit into from Dec 30, 2015

Conversation

Projects
None yet
2 participants
@BryceEWatson
Copy link
Contributor

commented Dec 23, 2015

For issue #171
Adds assign tag compiler, taglib, and test files.
The compiler uses builder.assignment() to write one or more assignments.

@patrick-steele-idem

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2015

Hey Bryce, thanks for the PR. What you submitted will work, but let's avoid introducing a new Assign AST node since it will confuse things (there is already an Assignment AST node). Instead, let's use the codeGenerator(elNode, generator) API for the custom tag to generate an array of assignment expressions. Something like the following should work:

module.exports = function codeGenerator(elNode, generator) {
    var builder = generator.builder;

    return elNode.attributes.map((attr) => {
        return builder.assignment(attr.name, attr.value);
    });
};

As an example, the <for> tag uses the code generator API: https://github.com/marko-js/marko/blob/htmljs-parser/taglibs/core/for-tag.js

Instead of using "node-factory" in the taglibs/core/marko-taglib.json you should use do the following:

"code-generator": "./assign-tag"

Also note, I submitted a bunch of changes to the htmljs-parser branch. Please make sure you pull in all of the latest changes! Thanks again!

@patrick-steele-idem

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2015

Hey @BryceEWatson, just wanted to check in and see if you had some time to try implementing the <assign> tag with the code-generator API. Please let me know if you would like more details. Thanks.

@BryceEWatson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2015

Hi @patrick-steele-idem, I notice that Assignment does not add semi-colons or new lines. Assuming I'll just add those in the tag file instead? Or would this be something to add into the Assignment builder?

@patrick-steele-idem

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2015

The AST Nodes do not need to write out the semicolons. That gets handled by the generateStatements() method:

if (!this._code.endsWith('\n')) {
this._code += ';\n';
}

If you just return an array of assignment nodes then everything should work as expected, but please verify.

@BryceEWatson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2015

Thanks @patrick-steele-idem, I've included your suggestion & squashed the commit. Let me know if there's any other improvements you see.

@patrick-steele-idem

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2015

Looks great. Glad everything works with less code. Thanks!

Let me know if you are interested in taking on the <include> tag next: #178

patrick-steele-idem added a commit that referenced this pull request Dec 30, 2015

@patrick-steele-idem patrick-steele-idem merged commit 257dd1f into marko-js:htmljs-parser Dec 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.