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 options.escape #11

Merged

Conversation

wolfgang42
Copy link
Contributor

I needed this because I was word-wrapping some text that would be embedded in an SVG document. If I escaped it before embedding (a) the lengths would have been wrong, and (b) an entity could be broken across two lines.

verb decided it wanted to modify some other stuff in the readme, so I let it.

verb decided it wanted to modify some other stuff in the readme, so I let it.
@jonschlinkert
Copy link
Owner

thanks! reviewing now

verb decided it wanted to modify some other stuff in the readme, so I let it.

ha, well if verb makes bad choices, please let me know lol. I'd love feedback

@@ -21,6 +21,8 @@ module.exports = function(str, options) {

var newline = options.newline || '\n' + indent;

var escape = options.escape || function(str){return str;};
Copy link
Owner

Choose a reason for hiding this comment

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

would you mind changing this to:

function identity(str) { 
  return str; 
};
var escape = typeof options.escape === 'function'
  ? options.escape
  : identity;

so we can explicitly ensure that escape will always be a function (and not true or something)

@jonschlinkert
Copy link
Owner

looks great, and it's a great idea. would you mind making the proposed changes? If not, that's okay I can merge as-is and make the changes - I'm grateful for the pr.

@wolfgang42
Copy link
Contributor Author

It's not that verb made bad choices, no. I just thought I'd explain why the diff of Readme.md had some unrelated changes in it.

@wolfgang42
Copy link
Contributor Author

@jonschlinkert Have you had a chance to look at the changes I made?

jonschlinkert added a commit that referenced this pull request Oct 1, 2015
@jonschlinkert jonschlinkert merged commit 0006ac4 into jonschlinkert:master Oct 1, 2015
@jonschlinkert
Copy link
Owner

thanks for the reminder!

@wolfgang42 wolfgang42 deleted the feature/option-escape branch October 7, 2015 14:09
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