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

Integrate new metal/soyparser #11

Merged
merged 5 commits into from
May 25, 2017
Merged

Integrate new metal/soyparser #11

merged 5 commits into from
May 25, 2017

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented May 23, 2017

This PR updates support to use the newer metal/soyparser

}

// Parse inner params
template.params.forEach(extractParam);
Copy link
Contributor Author

@jbalsas jbalsas May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mthadley, could template.params be null?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (cmd.deltemplate) {

parsed.body.forEach(function(template) {
if (template.deltemplate) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will always be false, since neither Template or DelTemplate have a deltemplate field. If the intent is to skip these, you can check if template.type === 'DelTemplate'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, yeah, didn't think about it. Will update it and add a test for it, thanks!

return;
}

var templateName = cmd.name;
var templateName = template.id.name;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just be aware that this will only be something like .render, and namespace is a separate attribute on id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True... I'll double check to see what were we getting before. Thanks!

@jbalsas
Copy link
Contributor Author

jbalsas commented May 24, 2017

Hey @mthadley, I've added a test for the delTemplate case (which of course was failing) and also caught the namespace issue you mentioned.

Could you please take a final look?

Thanks!

Copy link

@mthadley mthadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbalsas LGTM

@jbalsas jbalsas merged commit f15cd9f into master May 25, 2017
@jbalsas
Copy link
Contributor Author

jbalsas commented May 25, 2017

Merged, thanks @mthadley!

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

Successfully merging this pull request may close these issues.

2 participants