Set TM_MARKDOWN instead of duplicating preview command.

textmate / GitHub-Markdown.tmbundle

:octocat: GitHub Flavoured Markdown for TextMate

MIT License

40 stars 12 forks source link

Set TM_MARKDOWN instead of duplicating preview command. #10

Closed noniq closed 8 years ago

noniq commented 8 years ago

As discussed by @sorbits in http://lists.macromates.com/textmate/2016-July/039722.html this achieves the same result with less code. And it will also allow to use TM_MARKDOWN_FILTER for things like stripping front matter in the future.

MikeMcQuaid commented 8 years ago

Nice! Will need to test this out but seems like a big win.

MikeMcQuaid commented 8 years ago

:metal:

noniq commented 8 years ago

Let me know if you experience any problems. I’m already using this branch, for me everything seems to work fine.

MikeMcQuaid commented 8 years ago

I get "Preview: line 13: /Users/mike/Library/Application: No such file or directory" when testing this.

MikeMcQuaid commented 8 years ago

Looks like a variable that needs quoted?

noniq commented 8 years ago

Ah, of course … I think the preview command in the current version of TextMate's Markdown bundle has a quoting issue. https://github.com/textmate/markdown.tmbundle/pull/39 contains a fix but has not meen merged yet.

Not sure if my fix was entirely correct, though. @infininight, do we need to quote env vars when setting them from the bundle? If so, how?

MikeMcQuaid commented 8 years ago

@noniq Cool, can you give me a shout when that's merged and I'll retest? Thanks!

sorbits commented 8 years ago

Not quoting the variable was a deliberate change: textmate/markdown.tmbundle@13d3d314fcc51b8229782fcc555e6efa2c34ed78.

Either we quote the use of TM_MARKDOWN and thus drop support for providing extra arguments, or alternatively this commit needs to be changed to set TM_MARKDOWN to something like: eval '$TM_BUNDLE_SUPPORT/bin/redcarpet.rb'. This assumes that there is no single quote in the value of TM_BUNDLE_SUPPORT, a fairly safe assumption, but to be 100% safe it should probably be: eval '${TM_BUNDLE_SUPPORT/'/'\''/g}/bin/redcarpet.rb'.

What do you guys think?

noniq commented 8 years ago

Allowing for arguments make sense. I’ll update the PR to include quoting!

noniq commented 8 years ago

Updated! This PR should now work correctly even with the current state of the bundle support bundle.

(I also made a corresponding change to https://github.com/textmate/bundle-support.tmbundle/pull/19 where I reverted the additional quoting I had introduced before.)

MikeMcQuaid commented 8 years ago

Great, thanks @noniq!

MikeMcQuaid commented 8 years ago

@noniq Relatedly, because of your great work here when @sorbits added the support to repreview automatically on file changes this Just Works in the GitHub Markdown preview too without this needing any changes 🎉