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

add test: comment content colliding with variable #137

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

s9105947
Copy link
Contributor

Currently, an implementation treating comments as undefined variables
successfully passes all tests, because both undefined variables and
comments render to empty string.

This clarifies the behavior: A comment MUST NOT render into anything,
under any circumstances. This includes the case where a variable with
the same name as the comment content is defined.

The test data is designed in a way to trigger any possible name
collision, including whitespaces, a leading exclamation mark (!).

Closes #136

Currently, an implementation treating comments as undefined variables
successfully passes all tests, because both undefined variables and
comments render to empty string.

This clarifies the behavior: A comment MUST NOT render into anything,
under any circumstances. This includes the case where a variable with
the same name as the comment content is defined.

The test data is designed in a way to trigger any possible name
collision, including white spaces, a leading exclamation mark (!).
Copy link
Member

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

The test looks reasonable and thorough to me. 👍

My own implementation passes without modification.

  official mustache specification
    comments.yml
      ✔ ...
      ✔ Variable Name Collision

@gasche
Copy link
Contributor

gasche commented Mar 22, 2022

This also looks like a good idea to me. Thanks!

@jgonggrijp
Copy link
Member

@spullara or @Danappelxx I think this one is ready to merge.

@jgonggrijp
Copy link
Member

Also, I hereby offer to join you as a maintainer, to help process pull requests. No offense if you're not into it (yet).

@Danappelxx
Copy link
Collaborator

I've actually been trying to find your email to offer you the permissions 😁 my new job doesn't permit me to maintain open source projects so I'd love to hand it off!

@jgonggrijp
Copy link
Member

jgonggrijp commented Jul 20, 2022

That's a happy coincidence! If you managed to find it, please send me an email to tell me how you did it. :-)

Thanks for having me on board!

@jgonggrijp jgonggrijp merged commit f794f93 into mustache:master Jul 20, 2022
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.

Comments: Behavior on variable name collision
4 participants