atom / snippets

Atom snippets package
MIT License
204 stars 102 forks source link

filePath should be escaped for markdown in notifications #117

Open ruv opened 9 years ago

ruv commented 9 years ago

File name can be shown incorrectly in notification. The sequence \. is turned into . — for example, it is read as "\Users\user.atom\packages" instead of "\Users\user.atom\packages".

The issue is that atom.notifications.addError function interprets message as in Markdown markup. So, filePath should be properly escaped in a code like:

atom.notifications?.addError("Failed to load snippets from '#{filePath}'", {detail: error.message, dismissable: true})
izuzak commented 9 years ago

@ruv Thanks for the report. I'm guessing you wanted to open this issue on this repository instead https://github.com/atom/notifications? If that's so, could you close this one and reopen over there?

ruv commented 9 years ago

Although, the better way is to solve the issue via additional option, something like

atom.notifications?.addError("Failed to load snippets from '#{filePath}'", {detail: error.message, dismissable: true, contenttype: 'text/plain' })

PS @izuzak, I think that I have opened the issue in the right place.

izuzak commented 9 years ago

Ahhh, thanks for clarifying, @ruv -- it seemed you were making a general observation, because that pattern is used in some other packages, I believe.

izuzak commented 9 years ago

@benogle should we try and update all such calls we have in all packages or is there a smarter way to approach this? :thought_balloon:

benogle commented 9 years ago

We could escape the path before making it into the notification

ruv commented 9 years ago

@izuzak, regarding a smarter way... Maybe it is reasonable to set default contenttype option value to text/plain in Notification constructor in case if it is known that Markdown is never used in a message text that is dispatched via atom.notifications.*