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

Update elixir #2773

Merged
merged 17 commits into from Mar 2, 2021
Merged

Update elixir #2773

merged 17 commits into from Mar 2, 2021

Conversation

@gordalina
Copy link
Contributor

@gordalina gordalina commented Feb 20, 2021

Add support for @doc, @moduledoc, atom modules, function call tokens & raise keyword

@github-actions
Copy link

@github-actions github-actions bot commented Feb 20, 2021

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of +11 B (+1.3%).

file master pull size diff % diff
components/prism-elixir.min.js 833 B 844 B +11 B +1.3%

Generated by 🚫 dangerJS against bf45116

@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Feb 21, 2021

Could you please explain the motivation for this? The current highlighting seems to be quite good.

image

Code snippets for here and here.

@gordalina
Copy link
Contributor Author

@gordalina gordalina commented Feb 23, 2021

@RunDevelopment those two examples don't capture the extents of this change. I've put together a couple of examples to exemplify these changes.

Take for example this codepen and this gist - both have 3 examples, the codepen being the prismjs rendered one and the gist the one that github uses.

Prismjs does not parse @doc/@module doc as its own type, it currently returns string, which can't be syntax highlighted as a comment (see example of github).
This change also adds tokenization of modules and function calls, see in the gist example 3 line 25 and 26. Github tokenize Application and put_all_env separately, OTOH prismjs does not tokenize them, this PR tokenizes them.

This PR also adds the raise keyword.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Thanks for the clarification.

One thing aside from my comments:

The goal of the new doc rule is to enable GitHub-like highlighting, right? If that's the case, then you could also change the attribute rule to give @doc and @moduledoc a special alias. With that alias you could do GitHub-like highlighting in CSS like this:

.token.attribute.special-alias,
.token.attribute.special-alias + .token.string {
  color: #888; /* or whatever color you want */
}

No need to add a new doc rule.

The new doc rule also has a problem. Neither doc nor doc-comment are supported by Prism's standard themes. This means that doc attributes will remain unhighlighted despite being tokenized.

components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
@gordalina
Copy link
Contributor Author

@gordalina gordalina commented Feb 23, 2021

The goal of the new doc rule is to enable GitHub-like highlighting, right? If that's the case, then you could also change the attribute rule to give @doc and @moduledoc a special alias. With that alias you could do GitHub-like highlighting in CSS like this:

This makes a lot of sense, and it works with @doc one single line but when we have multiline docs .attribute + .string doesn't work as they're separated in different token lines. How do you propose to solve it?

@moduledoc """
multiline
documentation
"""

I'll push the changes you requested

gordalina and others added 6 commits Feb 23, 2021
Add support for @doc, @moduledoc, atom modules, function call tokens & raise keyword
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Feb 23, 2021

doesn't work as they're separated in different token lines

I forgot that some libraries that use Prism under the hood do that. Prism doesn't split code into lines by itself, so I didn't account for this. Yes, my proposed alternative solution doesn't work.

@gordalina
Copy link
Contributor Author

@gordalina gordalina commented Feb 24, 2021

@RunDevelopment I've updated it with your requests and with a suggested direction on how to deal with @doc/moduledoc.

Here's what I think is the right approach with the right compromise

  • Everything that's after @attr <...> is an elixir statement, so it should be tokenized as such. Case in point: @doc since: "1.3.0"
  • The elixir guide on writing documentation points out that it should be @(doc|moduledoc) "string" so I suggest we only tokenize those as docs and the remainder as normal attributes. (see included doc_feature.test)

Let me know what you think.

@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Feb 24, 2021

Sounds good! Let's go with that.

components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
components/prism-elixir.js Outdated Show resolved Hide resolved
tests/languages/elixir/capture_feature.test Show resolved Hide resolved
tests/languages/elixir/capture_feature.test Outdated Show resolved Hide resolved
components/prism-elixir.js Show resolved Hide resolved
gordalina and others added 6 commits Mar 1, 2021
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
@gordalina gordalina requested a review from RunDevelopment Mar 1, 2021
@gordalina
Copy link
Contributor Author

@gordalina gordalina commented Mar 1, 2021

@RunDevelopment I've addressed all your comments and it's ready for review.

components/prism-elixir.js Outdated Show resolved Hide resolved
gordalina and others added 2 commits Mar 1, 2021
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
@gordalina
Copy link
Contributor Author

@gordalina gordalina commented Mar 1, 2021

@RunDevelopment I've updated it, should be ready to go 🤞

@gordalina gordalina requested a review from RunDevelopment Mar 1, 2021
@RunDevelopment RunDevelopment merged commit e6c0d29 into PrismJS:master Mar 2, 2021
8 checks passed
@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Mar 2, 2021

Thank you for contributing @gordalina!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants