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.

Sign up for GitHub

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

Jump to bottom

Simplify multiple statements to a single line return statement #552

Closed
wants to merge 1 commit into from
Closed

Simplify multiple statements to a single line return statement #552

wants to merge 1 commit into from

Conversation

harryi3t
Copy link

@harryi3t harryi3t commented Mar 9, 2016

I took this PR
and modified it to make it work.

Ran the test cases before pushing. All passed

@harryi3t
Copy link
Author

harryi3t commented Mar 9, 2016

hey @phillipj,
could you look into this. Should I be pushing only one commit?

@phillipj
Copy link
Collaborator

phillipj commented Mar 9, 2016

Thanks, I'll have a look as soon as I've got some spare time.

Please squash the commits, especially since the first commit introduces
syntax errors IIRC.

On Wednesday, 9 March 2016, Harendra Singh [email protected] wrote:

hey @phillipj https://github.com/phillipj,
could you look into this. Should I be pushing only one commit?

—
Reply to this email directly or view it on GitHub
#552 (comment).

@harryi3t
Simplifies multiple statements into one
ce0659e 
We don't need these 3 statements.
We can simple return the output of context.lookup function.
Even it is null, returning null instead of underfined seems to have
no side effect.
@dasilvacontin
Copy link
Collaborator

Let me check if these changes provoke any edge case...

dasilvacontin added a commit that referenced this pull request Mar 15, 2016
@dasilvacontin
Assert null lookup when rendering an unescaped value
bcb89cc 
It was not being asserted anywhere else, and I was suspecting that
PR #552 was chaging current (and correct) behaviour.
dasilvacontin added a commit that referenced this pull request Mar 15, 2016
@dasilvacontin
Assert null lookup when rendering an unescaped value
c7a97ee 
It was not being asserted anywhere else, and I was suspecting that
PR #552 was changing current (and correct) behaviour.
@dasilvacontin dasilvacontin mentioned this pull request Mar 15, 2016
Merged
dasilvacontin added a commit that referenced this pull request Mar 15, 2016
@dasilvacontin
Assert null value render
22b7515 
It was not being asserted anywhere else, and I was suspecting that
PR #552 was changing current (and correct) behaviour.
dasilvacontin added a commit that referenced this pull request Mar 15, 2016
@dasilvacontin
Assert null value render
7e83df9 
It was not being asserted anywhere else, and I was suspecting that
PR #552 was changing current (and correct) behaviour.
@dasilvacontin
Copy link
Collaborator

I believe these changes introduce incorrect behaviour, but I'm waiting to confirm so: #553.

@dasilvacontin
Copy link
Collaborator

I don't really think we can simplify these lines, unless we want to provide a different rendering behaviour for values different than undefined and null. And the result wouldn't be simpler or less lines of code. 😄

Let me know if I'm mistaken, closing the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harryi3t @phillipj @dasilvacontin