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

added custom output function name #248

Merged
merged 4 commits into from
Apr 22, 2018
Merged

added custom output function name #248

merged 4 commits into from
Apr 22, 2018

Conversation

oscarotero
Copy link
Contributor

@oscarotero oscarotero commented Mar 27, 2017

According with #244 I've added a new option to define an output function name like echo or print. Example:

<h1><% echo('hello world') %></h1>
var ret = ejs.compile(string, {
    filename: path,
    outputFunctionName: 'echo'
})(data);

Any suggestion is welcome

@@ -1,7 +1,7 @@
<h1>Users</h1>

<% function user(user) { %>
<li><strong><%= user.name %></strong> is a <%= user.age %> year old <%= user.species %>.</li>
<li><strong><%= user.name %></strong> is a <%= user.age %> year old <% echo (user.species) %>.</li>
Copy link
Owner

Choose a reason for hiding this comment

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

Since echo is a function, there shouldn't be a space between the function name and the parens. Should look like echo(user.species);.

Also, rather than having this example mixed together with the output tag, might it not make sense to have a totally separate example?

var ret = ejs.compile(read(path, 'utf8'), {filename: path})(data);
var ret = ejs.compile(read(path, 'utf8'), {
filename: path,
outputFunctionName: 'echo'
Copy link
Owner

Choose a reason for hiding this comment

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

Same feedback as above -- since this is not likely the default use-case, it make make more sense to make this a separate example.

@mde
Copy link
Owner

mde commented May 7, 2017

I've added a couple of comments. Sorry for the delay in responding.

@oscarotero
Copy link
Contributor Author

Thank for the feedback. I've commited the changes.

@@ -0,0 +1,3 @@
<p>
<strong><%= echo(name) %></strong> is a <%= echo(age) %> year old <% echo(species) %>.
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like you neglected to change all tags to script-execution tags. In fact, you might want to make your example look more like your real use-case. For example, you might want to use output during debugging or something? You could even include a couple of examples of different ways to use it. It's good for people to know why this feature exists.

@mde
Copy link
Owner

mde commented May 7, 2017

One more comment .... :)

@mde
Copy link
Owner

mde commented May 7, 2017

Also, did you verify if it plays nicely with the line-numbers in errors?

@oscarotero
Copy link
Contributor Author

Ok, now it has a more realistic example.
and about the line-numbers in errors, this is an example:

    1| <ul>
 >> 2| <%
    3|     users.forEach(function (user) {
    4|         echo(a + '<li>');
    5|             echo('<strong>' + user.name + '</strong>');

a is not defined

as you can see, the error is in line 4, not 2, not sure whether it's a problem with echo.

@mde
Copy link
Owner

mde commented May 9, 2017

Try your same error without the echo call, and see if the line number in the error is correct.

@oscarotero
Copy link
Contributor Author

Same error:

    1| <ul>
 >> 2| <%
    3|     users.forEach(function (user) {
    4|         a = b;
    5|             echo('<strong>' + user.name + '</strong>');

b is not defined

@mde
Copy link
Owner

mde commented May 10, 2017

Ah, might be the line number for the opening tag. It's been a long time since I touched that code. I'll play around with it and see what it's doing to make sure this doesn't confuse things.

@oscarotero
Copy link
Contributor Author

Hi.
I've added an example combining echo and include: 9252132

Any plans for a new release including this feature?

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

Code LGTM, needs docs yet.

@mde @User4martin Thoughts on this?

@User4martin
Copy link

Should also include a test case.

@mde @RyanZim
IMHO it would be better (more consistent with existing code) if "echo" would be passed as param to the function.

In case of opts.client it can have be tested for undefined, and set to __append in that case.

@sziberov
Copy link

Any changes on this? Really want this feature, as I several days ago switched to Node from PHP and having similar syntax would be amazing for me.

@ViktorQvarfordt
Copy link

Is this not sufficient?

<% for (const user of users) { %>
  <li><%= user.name %></li>
<% } %>

@mde mde merged commit 9252132 into mde:master Apr 22, 2018
@mde
Copy link
Owner

mde commented Apr 22, 2018

Sorry for the lengthy delay on this. I've verified there's no change to line numbers in errors. Thanks very much for this!

@oscarotero
Copy link
Contributor Author

Great!

@iagobruno
Copy link

@ViktorQvarfordt is more readable.

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.

7 participants