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

Don't ask escaping for boolean attributes at the beginning of attributes-list #2956

Merged
merged 2 commits into from Feb 12, 2018

Conversation

ezhlobo
Copy link
Member

@ezhlobo ezhlobo commented Feb 11, 2018

Each attribute without value has true value by default. It should not be 'escaped' because it happens internally. But when we place attribute without a value at the beginning, we have a bug when mustEscape is set to true.

This PR prevents this behavior.

Example

Current

div(first, second, third)

//- first  -> mustEscape: true
//- second -> mustEscape: false
//- third  -> mustEscape: false

Next

div(first, second, third)

//- first  -> mustEscape: false
//- second -> mustEscape: false
//- third  -> mustEscape: false

@ezhlobo
Copy link
Member Author

ezhlobo commented Feb 11, 2018

Maybe I understood this flag wrongly, and instead of false it should be true. Could you give me feedback, please?

@jbsulli
Copy link
Contributor

jbsulli commented Feb 11, 2018

Yeah, I've observed this behavior as well. We had a discussion on it here: pugjs/pug-lexer#76 (comment). The pull request that I am working on is going to make all of the attributes without a value default to true based on their feedback.

@ezhlobo
Copy link
Member Author

ezhlobo commented Feb 12, 2018

@jbsulli oh, thank you for letting me know. I do need this fix to improve other project so what do you think: should I wait for your changes or make this PR appropriate?

@ForbesLindesay ForbesLindesay merged commit eedf9a3 into pugjs:master Feb 12, 2018
@ForbesLindesay
Copy link
Member

Thanks, I think consistency is the main thing here, and I think it's fine to mark these as unescaped.

@ezhlobo
Copy link
Member Author

ezhlobo commented Feb 12, 2018

@ForbesLindesay hey, are you planning to publish this fix?

@ForbesLindesay
Copy link
Member

I'm going to wait for #2957 as I think we might flip to marking all these attributes as "mustEscape: true".

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

3 participants