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

Preserve indentation when rendering partials #703

Closed
wants to merge 5 commits into from
Closed

Preserve indentation when rendering partials #703

wants to merge 5 commits into from

Conversation

yotammadem
Copy link
Contributor

A fix for: #562

@yotammadem
Copy link
Contributor Author

not all spec tests are passing, closing...

@yotammadem yotammadem closed this Jun 25, 2019
@yotammadem yotammadem reopened this Jun 26, 2019
@yotammadem
Copy link
Contributor Author

yotammadem commented Jun 26, 2019

I've added all the tests described in this spec: https://github.com/mustache/spec/blob/master/specs/partials.yml
And they are passing now
@janl, @mjackson @dasilvacontin - If one of you (or anyone else from the team) can review and merge/reject it would be great

@yotammadem
Copy link
Contributor Author

I decided to follow the original solution of @kevindew (PR: #613) as it looks more elegant
I had to change some minor things to make it work according the the spec tests

@phillipj
Copy link
Collaborator

phillipj commented Jul 8, 2019

This looks really promising! Adding the tests from the mustache spec project was a great addition 👍

I really feel @kevindew should get more credit for his contributions, as he wrote the first draft in #613 as you mention. At the moment, none of the commits has his name on it, which isn't quite fair in my eyes. Wondering if we should use his changes as the first commit, then add your commits as tweaks on top. That way you both get credit for your contributions, well deserved 😄

@yotammadem
Copy link
Contributor Author

@phillipj
Sorry,
I didn't think about the commits/credit aspect, I just wanted the thing to work.
I will re-arrange the PR such that it will include Kevin's commits

@phillipj
Copy link
Collaborator

phillipj commented Jul 8, 2019

Fully understandable!

Cool 👍

@yotammadem yotammadem closed this Jul 9, 2019
@yotammadem
Copy link
Contributor Author

yotammadem commented Jul 9, 2019

@phillipj
Done - please look at
#705

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.

None yet

2 participants