Use Textmate's HAML grammar #3627
Conversation
How do I review a license? :) |
|
When you say "unbounded nesting", which lines are you referring to? I've only taken a cursory glance through the current HAML grammar, but I see a few oddities:
I doubt any of these errors are what're causing the nesting errors. May I see exactly which lines in the file you linked to are being directly affected? |
|
Regarding the license review: I believe our approach is to whitelist the license file's checksum in @pchaigno Should the hash list be moved to a config file in |
|
Further to @Alhadis's comments, it appears the original author (@meganemura) of the switch (#3355) soon encountered issues with the grammar they implemented (see #3371) and asked for this to be reverted but it was missed. With that in mind, I think it may be worth reverting (this PR) to address #3371 and https://github.com/github/github/issues/68429 and https://github.com/github/github/issues/72875 As for the failing test: the license that has been detected (other) is none of those on the whitelist. This was "permissive" when we last used this grammar as we can see in #3355. I'm not sure how it got that though. |
|
Oh! I forgot all about that... Alright, in that case, revert the hell out of this. =) |
|
@Alhadis: Just for completeness, you can see how https://github.com/ezekg/language-haml/blob/master/grammars/ruby%20haml.cson#L408 includes the original pattern again (i.e. recursively) when hitting a chunk of inline Ruby code. The result is that if you have several lines in a row, each line contains all the tags from the previous lines. It's very obvious in the HAML file I posted because of the huge chunks of redundant HTML in the page: That is one line; the following line has the same contents + 2 extra tags, and the following one +4, and so on. @lildude: Fixed the licensing. Can you please approve? |
|
Thank you @Alhadis! |
|
@bighairydude beat @lildude. =) You're most welcome. |
It was supposed to be only temporary, but I'm having a hard time getting rid of these unrecognized licenses. I suppose we could move the whitelists to their own file. @vmg Has the issue been reported to the |
|
I'll take a break from developing a graphical troff emulator for Atom to have a look at what's wrong... |
|
Already fixed. How's this look? |
I wouldn't trust it 😉. Lightshow is currently running Linguist 4.3.1 😱 I'll be updating it later. |
|
@Alhadis Dayum, that works like a charm. Highlighting looks great and it doesn't create any redundant nodes. I've verified it using our production highlighting engine. 🙏 Could you please upstream that? We can then revert this PR right away. |
|
Already on it. =) |
|
|
|
Upstream fix has been merged (🙇 @Alhadis) so I'm going to revert this and then update the grammar SHA. |
|
@Alhadis: Since I have you here (he hehehe), I've just noticed another bug on that grammar. It's basically #3371 -- where some string interpolations with I'm afraid your changes didn't catch that one. If you have some spare time to look into that, that'd be n e a t o. ✨ |
|
Sorry! I was AFK (which happens every now and then). I'll tackle it now. |
|
Is it possible to update the version of Linguist used by Lightshow? Or is that a bit more complicated than punching in a few commands? Both Atom and Lightshow generate the same output, which is different to what's showing on GitHub. Which kinda makes it impossible to tell if changes will actually fix anything... Other than Lightshow, is there anything else I can use for previewing highlighted output? |
As simple as updating a Gemfile and redeploying 😄 I was going to wait for the next linguist, but I can do it now if it helps you. |
|
Yes please! =) That'd be ace. |
|
@lildude Please ping me once it's done. I'm afraid there's very little I can do in the meantime. |
|
@Alhadis will do. Things are so far behind that it's taking a little more than updating a Gemfile to solve. Will let you know when it's good to go. |
|
@Alhadis Done. Lightshow is now running the same version of the parser and linguist (5.0.9) as we're running on github.com. I've also added a little tweak to the interface to show the version of Linguist so peeps can determine this easily and give us a prod when we forget to update Lightshow 😄 If you have the time, and are able to sort out the HAML issue and get it merged by the end of the day on Thursday, that would be amazing. |
|
Hey all! Maintainer of the Haml language package jumping in here—so I'm curious as to why GitHub renders the grammar differently than Atom? Running this test code through Lightshow (and of course on GitHub) renders very different results than Atom. This is what it looks like in Atom (the whole file renders is correctly),
Is the GitHub parser vastly different than the one you're using in Atom? Let me know if I need to dig in and fix things with the Haml grammar—when I took over that language package I didn't know a whole lot about grammars. After taking it over, I went in and closed out a lot of the issues that had been lingering in the Textmate grammar (mostly multiline stuff), but most of that was honestly by trial and error playing with different regexes. tldr; I knew regex, but not how to put together a grammar so it may be incorrect/messy. |
|
Pushed what I believe will be a fix. It's a silly kludge but it fixes the interpolation highlighting:
|
@ezekg it is quite different so there can be some slight differences. @Alhadis is probably the most knowledgable when it comes to getting grammar working correctly with the GitHub parser. I think to keep this alive and getting the attention it needs, @ezekg can you move this to a new issue? 🙇 |
The
language-hamlsyntax for HAML has a recursive include (https://github.com/ezekg/language-haml/blob/master/grammars/ruby%20haml.cson#L360-L408) which is causing unbounded nesting of tags when parsing some HAML documents.As an example, see the HTML source for this rendered document: https://github.com/ManageIQ/manageiq-ui-classic/blob/9b4bfe77184a177f24159fce36cd9377a2f020da/app/views/layouts/angular/_multi_auth_credentials.html.haml
It's not clear to me how to fix the HAML grammar, but I really believe the recursive inclusion is not correct. Hence, I propose we rollback to TextMate's grammar, which does not have this issue.
cc @github/linguist
Fixes https://github.com/github/github/issues/72875