atom / language-gfm

GitHub Flavored Markdown in Atom
MIT License
101 stars 107 forks source link

Adding syntax highlighting support for Liquid tags #145

Open fusion809 opened 8 years ago

fusion809 commented 8 years ago

Hi,

I use this package quite frequently when writing posts in my Jekyll site (The Hornerysource) and it would help big time if this package had syntax-highlighting and autocompletion support for Liquid tags. If it helps, there is a language-liquid package available for Atom, it has syntax-highlighting support for HTML files with Liquid tags in it.

Thanks for your time, Brenton

burodepeper commented 8 years ago

I think this is best solved by adding markdown files to the filetypes recognized by the language-liquid package (see: https://github.com/kieranmasterton/language-liquid/blob/master/grammars/html%20(liquid).cson#L1-L4). This way language-liquid can inject its functionality into other grammars. (cc @kieranmasterton)

Another solution would be to include language-liquid as part of language-gfm (this is how I support html in my language-markdown package), but I'm not sure if that gives any problems for people that don't have the liquid package installed.

fusion809 commented 8 years ago

@burodepeper Ah, I just tried that and it failed. See instead the GFM syntax highlighting was removed entirely from the md file I was editing and language-liquid was used in its place.

burodepeper commented 8 years ago

@fusion809 Ah yes, you are probably right; of source it only uses a single grammar at the time. My bad.

It shouldn't be too difficult to add it similar to this: https://github.com/burodepeper/language-markdown/blob/master/grammars/language-markdown.json#L1500-L1506 Do you have experience with this? Otherwise I'll add it to language-markdown and then they/you can copy my approach from there.

fusion809 commented 8 years ago

Na I'm afraid my knowledge of programming is limited. So that would be a big help, thanks.

burodepeper commented 8 years ago

No problem. Could you create an issue over here and supply me with some liquid content so I can test if it works?

fusion809 commented 8 years ago

Shall do.

fusion809 commented 8 years ago

@burodepeper Thanks, it's just is there anyway to translate your language-markdown changes into config.cson changes to get language-gfm to use language-liquid syntax highlighting for Liquid tags? Your package is pretty good, but what can I say I'm a language-gfm fan.

puranjayjain commented 8 years ago

@burodepeper I have got a list of snippets related to liquid and would like to add snippets for them. where should I send the PR?

burodepeper commented 8 years ago

@puranjayjain If they're purely related to liquid, I'd say send the PR to https://github.com/kieranmasterton/language-liquid

burodepeper commented 8 years ago

@fusion809 You can create a PR that adds this little bit to the language-gfm grammar. Not literally btw, the grammar only needs to include the text.html.liquid grammar and you're set.

puranjayjain commented 8 years ago

@burodepeper thanks but that repository seems to be dead as there is no update for the past 11 months, some issues and PRs are still open. What can be done about that? Any other suggestions or help would be appriciated

burodepeper commented 8 years ago

@puranjayjain Have you tried contacting the maintainer for the possibility of transferring ownership to yourself? According to the package.json the code is released under the MIT license, so if @kieranmasterton (ping, anyone home?) remains unresponsive, you could fork the project, and it release it as a new package.

puranjayjain commented 8 years ago

@burodepeper I had contacted the maintainer on his email id but seems there was no response. I'll try to contact him on twitter, if not I'll fork it and start a new package (which I'm not really sure if I want to). If I do start it can I add you and @fusion809 to it as co-maintainers i won't be very available for it all the time so.

burodepeper commented 8 years ago

@puranjayjain I hope it works out via Twitter. I agree that forking and publishing as a new package is more of a last resort. I'm not personally invested in liquid, but I don't mind helping out. It looks rather straight forward.

fusion809 commented 8 years ago

My understanding of CoffeeScript and JS is so limited, especially when it comes to Atom packages, that I wouldn't be of much help. I'm more helpful in suggesting features than anything else. I wouldn't even know how to implement the necessary changes to this package (I know @burodepeper gave me that file link to indicate something about what needs to be done, but my knowledge of Atom packages isn't really enough for me to know what to do with that file I have tried to fork this package locally and work on it but I just don't understand CoffeeScript/JS enough to really follow its code).

burodepeper commented 8 years ago

@fusion809 @puranjayjain I don't mind maintaining the package if the two of you aren't comfortable with that.

puranjayjain commented 8 years ago

@burodepeper @fusion809 I'm waiting for the author's response and am giving him a grace period of 24 hours to respond after which I'll start a fork and post here for any updates or ask you to start a fork and that's it I guess for now.

burodepeper commented 8 years ago

@puranjayjain 24 hours is a bit short. Give it at least a week. Forking only directly helps yourself and isn't in the interest of the already established user group.

puranjayjain commented 8 years ago

@burodepeper you are right I'll give him a week's period, sorry for the hassle :disappointed_relieved:

burodepeper commented 8 years ago

@puranjayjain No problem. Just actively trying to avoid unnecessary stress ; ) We'll talk later.

fusion809 commented 8 years ago

@puranjayjain @burodepeper Unless I'm mistaken it's been two weeks, anyone gonna start working on an improvement for this package? Sorry if I come across as bossy, it's just these features would really help me at least and I doubt I'm the only one that would benefit from them.

puranjayjain commented 8 years ago

@fusion809 sorry for taking so long i'm creating a fork of the language liquid package and adding @burodepeper to it and then publishing it to atom.

fusion809 commented 8 years ago

@puranjayjain You sure you should have forked language-liquid and not language-gfm? As you're adding Liquid support to the language-gfm package, not making adjustments to the language-liquid package.

puranjayjain commented 8 years ago

@fusion809 Sure since it is part of liquid language extending the gfm sytax. We'll include gfm in it to extend the support and to gain advantage of color highlighting. Similar to a lot of packages like template packages e.g language-nunjucks which adds support for nunjucks but on top of html

puranjayjain commented 8 years ago

@fusion809 I have left out some prs so you can add what you think is relevant. Now moving the conversation to the relevant repo.

burodepeper commented 8 years ago

@puranjayjain No, I think @fusion809 is correct. For his request, you don't need to fork language-liquid. That's a separate issue. Just create a PR for language-gfm that includes language-liquid (you can copy this from language-markdown) and you are set for now.

You don't want to include language-gfm in language-liquid.

puranjayjain commented 8 years ago

@burodepeper @fusion809 ok can you review my latest commit there and tell me if I can go ahead with a PR here? i'm only talking about the snippet file

burodepeper commented 8 years ago

@puranjayjain Sure, if you create a PR for language-gfm first.

puranjayjain commented 8 years ago

@burodepeper @fusion809 you mean like this? https://github.com/burodepeper/language-markdown/blob/master/grammars/language-markdown.json#L122

fusion809 commented 8 years ago

Precisely, sorry for being unclear, I honestly thought we were all on the same page.

puranjayjain commented 8 years ago

@fusion809 @burodepeper I was just now added to the original repo of language-liquid so it is now being maintained again.

burodepeper commented 8 years ago

@fusion809 @puranjayjain created a PR in #154.