A better solution for PR 129 #171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The [PR 129](#129) fixed issue 119, but would lost the include line's error information(such as his sample, we can't get error 'ejs/sample/index.html:18'). On the other way, his solution would make error filename wrong when you use both <%include('header.html')%> and <%-include body.html%>. So I think this solution makes things better. After using this PR, as his sample, the error information displays as bellow. ReferenceError: /Users/Spikef/ejs/sample/index.html:18 16| this_variable_is_not_defined is not defined 17| --> >> 18| <% include body.html %> 19| </body> 20| </html> /Users/Spikef/ejs/sample/body.html:2 1| <!-- Execute an exception --> >> 2| <p><%= this_variable_is_not_defined %></p>
|
I'll check this out when I get time. If @TimothyGu beats me to it, that's fine. |
|
LGTM; CC @mde & @TimothyGu for peer review. We will probably never get perfect error handling, but this helps. |
|
Thanks! |
|
BTW @mde, when are you planning for the next release? |
|
Thanks for the merge. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR 129 fixed issue 119, but would lost the include line's error information(such as his sample, we can't get error 'ejs/sample/index.html:18'). On the other way, his solution would make error filename wrong when you use both <%include('header.html')%> and <%-include body.html%>.
So I think this solution makes things better. After using this PR, as his sample, the error information displays as bellow.