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

[#1769] Fix for Clojure code block #1770

Merged
merged 4 commits into from Jul 14, 2020
Merged

Conversation

drbobbeaty
Copy link
Contributor

There was an issue in that the rendering of the Clojure code block in
a Markdown file would not be rendered properly. This is documented in
Issue #1769 - with screen grabs. This addition to the syntax file
corrects that issue by adding the section for the Clojure code block in
the Markdown file.

There was an issue in that the rendering of the Clojure code block in
a Markdown file would not be rendered properly. This is documented in
Issue sublimehq#1769 - with screen grabs. This addition to the syntax file
corrects that issue by adding the section for the Clojure code block in
the Markdown file.
Copy link
Collaborator

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should have taken a quick look at the other entries in this context instead of just copying what @OdatNurd provided 😉

Markdown/Markdown.sublime-syntax Outdated Show resolved Hide resolved
Markdown/Markdown.sublime-syntax Outdated Show resolved Hide resolved
@OdatNurd
Copy link

Indeed! My fix was based on the shipped version of the syntax in the build you mentioned using, which is somewhat different from the version currently in this repo, which has unreleased changes. 😃

FichteFoll and others added 2 commits November 21, 2018 05:07
Co-Authored-By: drbobbeaty <bob@bobbeaty.com>
Co-Authored-By: drbobbeaty <bob@bobbeaty.com>
@drbobbeaty
Copy link
Contributor Author

You guys are right - I should have... I'm terribly sorry about that.

@keith-hall
Copy link
Collaborator

Please add some syntax test assertions to ensure the changes you made work properly and won't be regressed in the future.

https://github.com/sublimehq/Packages#pull-requests

Pull requests should:

  • Have multiple syntax tests

See https://www.sublimetext.com/docs/3/syntax.html#testing for details.
This can be done easily with https://packagecontrol.io/packages/PackageDev.

It makes sense to add a test in the same spirit as the other code block
rendering tests, and this is something I pulled from the Clojure syntax
test, and adapted to the code block needs. It appears to match the
patterns, and the testing requests.
@wbond
Copy link
Member

wbond commented Nov 21, 2018

I'm going to hold off on this for now. The Markdown syntax grows in the number of contexts with each syntax we add. We are seeing the sanity limit hit more frequently due to the giant include graph.

@drbobbeaty
Copy link
Contributor Author

@wbond is there something wrong with the PR that can correct this? Or is it something else that is beyond this PR?

@wbond
Copy link
Member

wbond commented Nov 21, 2018

No, there is nothing wrong with this PR. It is more an issue of us needing to figure out one of:

  • How to try and detect which syntax may be causing the sanity limit
  • Increase the sanity limit some, profiling memory usage to see if something like 50,000 is too high
  • See if there is more significant modifications that can be made to the highlighting engine to not require compiling each external include into a syntax. This is most likely not viable, since the increase of contexts is usually due to with_prototype being used, creating new contexts with prepended rules.

@deathaxe
Copy link
Collaborator

Loading markdown files is already very slow, which makes me doubt an increased sanity limit to be a good solution in manner of performance.

@wbond wbond added the on hold Something that can't be done, or done correctly, right now label Oct 24, 2019
@wbond
Copy link
Member

wbond commented Jul 14, 2020

Since build 4075 we now have embed lazy loads, so this can be merged in without a performance penalty.

@wbond wbond merged commit c639f3f into sublimehq:master Jul 14, 2020
mitranim pushed a commit to mitranim/Packages that referenced this pull request Mar 25, 2022
Adds support for Clojure to Markdown fenced code blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Something that can't be done, or done correctly, right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants