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

Numbers in embedded cpp markdown are not highlighted #97

Closed
jeff-hykin opened this issue Apr 14, 2019 · 16 comments
Closed

Numbers in embedded cpp markdown are not highlighted #97

jeff-hykin opened this issue Apr 14, 2019 · 16 comments
Labels
External Depends on an external issue to be resolved low priority
Projects

Comments

@jeff-hykin
Copy link
Owner

jeff-hykin commented Apr 14, 2019

This is inside of a markdown document.

Screen Shot 2019-04-14 at 6 12 44 PM

There is this intersting issue where numbers are not tagged.
newPattern(
            match: /\b\d+\b/,
            tag_as: "constant.numeric"
        ),

Adding the pattern^ above the full number pattern actually fixes the issue (but it obviously breaks the complex number matching). My guess is that the number pattern is timing out, which I find surprising. The pattern is really long, but it should be able to match a two digit number in a very short amount of time. I'm also not exactly sure why it behaves different when it is embedded.

@matter123
Copy link
Collaborator

matter123 commented Apr 14, 2019

Does embedded actually use the extension or does it use the default (1.4.5 1.33.0, atom/language-c 1.33.1, 1.7.6 insiders)?

Edit: looks from your response that its yes.

@jeff-hykin
Copy link
Owner Author

Thankfully it uses the extension.

@jeff-hykin jeff-hykin added 🔍 investigating More information is being gathered low priority labels Apr 14, 2019
@matter123
Copy link
Collaborator

It looks significantly worse for me.
Screenshot from 2019-04-14 16-24-39

@jeff-hykin
Copy link
Owner Author

Yeah, pull from master I push pushed a large fix that was thanks to replacing all instances of $base

@jeff-hykin
Copy link
Owner Author

The issue with $base is that it is dynamic, it actually re-includes everything in the markup language, when really we wanted it to only be C++

@matter123
Copy link
Collaborator

Why was #cpp_base chosen over $self?

@jeff-hykin
Copy link
Owner Author

I believe that $self references the current pattern (so its like recursion) but $base always goes to the root most pattern.

@matter123
Copy link
Collaborator

matter123 commented Apr 15, 2019

I am fairly sure that is incorrect.

From https://macromates.com/manual/en/language_grammars

To reference the grammar itself, use $self:

From https://www.apeth.com/nonblog/stories/textmatebundle.html

The value of an include rule can also be $self, meaning the whole current grammar, or $base, the grammar inside which we are embedded. They are used by several prominent grammars (such as the Objective-C grammar), but I have never used them and I don’t quite understand the details.

Additionally using $self
Screenshot from 2019-04-14 17-05-02

@jeff-hykin
Copy link
Owner Author

oops, yup looks like we should switch to using $self. That is a terribly unintuitive naming scheme, good catch on your part though. I'm out of time for today, but the next change I do will probably be that.

The C grammar still needs the fix.

@matter123
Copy link
Collaborator

I can switch both to self in an hour or so.

@matter123
Copy link
Collaborator

Here is the vscode-textmate's inspect tool output for line 6 of example.md https://gist.github.com/matter123/fd7fbb3d2fd575208fb872058dc98b60.

And for the simple pattern https://gist.github.com/matter123/63ad7294e30926ad254ccfe84bc932e4

@matter123
Copy link
Collaborator

This is fixed by microsoft/vscode-textmate#88

@matter123 matter123 added the External Depends on an external issue to be resolved label Apr 16, 2019
@jeff-hykin
Copy link
Owner Author

jeff-hykin commented Apr 16, 2019

Thanks for looking into this so deeply. Thats a really nice inspect tool. I'll rename this issue so that people can find it easier.

That is also a really good catch with the objective-c dependency, I have no idea how you found that. Looks like we can't fix embedded c until we also change objective-c. Whenever I get a chance to work on this again I might go ahead and create an objective c repo just for the embedding fix.

@jeff-hykin jeff-hykin changed the title Embedded C++ might be timing out Numbers in embedded cpp markdown don't work Apr 16, 2019
@jeff-hykin jeff-hykin changed the title Numbers in embedded cpp markdown don't work Numbers in embedded cpp markdown are not highlighted Apr 16, 2019
@matter123 matter123 added the 🔵 Blocked Depends on another issue label Apr 18, 2019
@jeff-hykin jeff-hykin removed the 🔍 investigating More information is being gathered label Apr 18, 2019
@jeff-hykin jeff-hykin added this to Misc in Main Jun 25, 2019
@matter123
Copy link
Collaborator

vscode-textmate/#88 was merged in today. I'll close this as soon as insiders has the new version, and I confirm its fixed.

@jeff-hykin
Copy link
Owner Author

jeff-hykin commented Jul 12, 2019

Great, that means we can probably move forward with the almost-perfect preprocessor conditional pattern for #31

@matter123 matter123 removed the 🔵 Blocked Depends on another issue label Jul 12, 2019
@matter123
Copy link
Collaborator

With

Version: 1.37.0-insider
Commit: 894056d11c7fffb261a0100fd4dcbf8cff84a8c0
Date: 2019-07-15T05:25:26.901Z
Electron: 4.2.5
Chrome: 69.0.3497.128
Node.js: 10.11.0
V8: 6.9.427.31-electron.0
OS: Linux x64 5.2.0-gentoo snap

The numbers have correct highlighting.

Main automation moved this from Misc to Up Next Jul 16, 2019
@matter123 matter123 moved this from Up Next to Complete in Main Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Depends on an external issue to be resolved low priority
Projects
Main
  
Complete
Development

No branches or pull requests

2 participants