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

stringify description #37

Closed
wants to merge 1 commit into from
Closed

stringify description #37

wants to merge 1 commit into from

Conversation

dmlittle
Copy link

What

  • Escape quotes from answers.description before passing it to egad's scaffold function.

Why

Currently description isn't escaped in the probot template to allow for symbols in the description (<, > or &). This has the adverse effect that if double quotes are used in the description we end up with an invalid JSON file in package.json as they aren't escaped.

For example:

{
  "description: "a description with "quotes"."
}

Since it seems we purposely chose to not HTML escape descriptions I'm stringifiying the description and removing the beginning and end quotes of the JSON stringification in order to end up with a valid JSON object.

Happy to talk about other way of tackling this issue!

Misc

Closes probot/probot#727

Copy link
Contributor

@hiimbex hiimbex left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @dmlittle! I definitely appreciate the thought that went into this. However, I'm not really convinced this is the right way to handle the original issue. If we want to stringify or escape the description, I'd think we'd want to handle all of the variables in the template.

I left more thoughts in probot/probot#727 (comment) about the original source of this issue, which I'd like more clarity on before moving to a solution.

(Sorry to not merge what looks like a nice 1 line PR 😓)

@dmlittle
Copy link
Author

Closing this PR! As discussed in probot/probot#727 we'll just start escaping the description and author fields again in the template.

@dmlittle dmlittle closed this Oct 18, 2018
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.

2 participants