Skip to content

Added support for ReScript - #3435

Merged
RunDevelopment merged 20 commits into
PrismJS:masterfrom
vmarcosp:feat/rescript-support
Apr 15, 2022
Merged

Added support for ReScript#3435
RunDevelopment merged 20 commits into
PrismJS:masterfrom
vmarcosp:feat/rescript-support

Conversation

@vmarcosp

@vmarcosp vmarcosp commented Apr 15, 2022

Copy link
Copy Markdown
Contributor

Added support for ReScript

ReScript Prism Preview 1
ReScript Prism Preview 2
ReScript Prism Preview 3

@github-actions

github-actions Bot commented Apr 15, 2022

Copy link
Copy Markdown

JS File Size Changes (gzipped)

A total of 3 files have changed, with a combined diff of +815 B (+16.1%).

Details
file master pull size diff % diff
components/prism-rescript.min.js 0 Bytes 803 B +803 B +100.0%
plugins/autoloader/prism-autoloader.min.js 2.4 KB 2.41 KB +6 B +0.2%
plugins/show-language/prism-show-language.min.js 2.67 KB 2.67 KB +6 B +0.2%

Generated by 🚫 dangerJS against 3bd9b37

@fdaciuk fdaciuk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎉

@RunDevelopment RunDevelopment left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the PR @vmarcosp!

I gave the language a quick review and found a few things.

Comment thread components.json
Comment thread components/prism-rescript.js
Comment thread components/prism-rescript.js Outdated
Comment thread components/prism-rescript.js Outdated
Comment thread components/prism-rescript.js Outdated
Comment thread components/prism-rescript.js Outdated
Comment thread tests/languages/rescript/comment_feature.test
@vmarcosp

vmarcosp commented Apr 15, 2022

Copy link
Copy Markdown
Contributor Author

@RunDevelopment some tests are broken, I'll fix and ping you back soon.

@fakenickels

Copy link
Copy Markdown

🔥🔥🔥

Comment thread components/prism-rescript.js Outdated
@RunDevelopment

Copy link
Copy Markdown
Member

I'll be fix and ping you back soon

Take your time. I'm currently reading through the docs, so I'll probably add a few more comments.

Comment thread examples/prism-rescript.html Outdated
Comment thread components/prism-rescript.js Outdated
Comment thread components/prism-rescript.js Outdated
Comment thread components/prism-rescript.js Outdated
@RunDevelopment

Copy link
Copy Markdown
Member

Could we also add support for ReScript's char syntax? It could be done like this:

'comment': /.../,
'char': { pattern: /'.'/, greedy: true },
// ...

Also, chars seem to be heavily discouraged in the docs. Will this be removed eventually? The syntax does seem to somewhat overlap with generics.

Comment thread components/prism-rescript.js Outdated
@vmarcosp

Copy link
Copy Markdown
Contributor Author

Could we also add support for ReScript's char syntax? It could be done like this:

'comment': /.../,
'char': { pattern: /'.'/, greedy: true },
// ...

Also, chars seem to be heavily discouraged in the docs. Will this be removed eventually? The syntax does seem to somewhat overlap with generics.

Looks nice, I'll add.
Probably will be removed in the future, but since it's simple, I can add.

Comment thread tests/languages/rescript/class-name_feature.test Outdated
@RunDevelopment

Copy link
Copy Markdown
Member

Another nice feature would be to highlight boolean literals. Please add 'boolean': /\b(?:false|true)\b/ after number.

@RunDevelopment

Copy link
Copy Markdown
Member

I have a question about regexes. Is %re("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/b/g") special syntax, or is %re just a weird function name?

Comment thread components/prism-rescript.js Outdated
@vmarcosp

Copy link
Copy Markdown
Contributor Author

I have a question about regexes. Is %re("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/b/g") special syntax, or is %re just a weird function name?

%re Is a ppx, but we can highlight this one as a function.

@RunDevelopment

Copy link
Copy Markdown
Member

Are all PPX of the form %name? If so, then we could highlight all of them as a function (or similar).

Comment thread tests/languages/rescript/char_feature.test Outdated
@vmarcosp

Copy link
Copy Markdown
Contributor Author

Are all PPX of the form %name? If so, then we could highlight all of them as a function (or similar).

There are three forms of ppx:

  • @ like @react.component - This one I'm highlighting as a class-name because it behaves like a decorator
  • %% like %%raw - This one we can highlight as a function because it behaves very similar to a function.
  • % like %re - The same as %%

Comment thread tests/languages/rescript/operator_feature.test Outdated
@RunDevelopment

Copy link
Copy Markdown
Member

There are three forms of ppx:

  • @ like @react.component - This one I'm highlighting as a class-name because it behaves like a decorator
  • %% like %%raw - This one we can highlight as a function because it behaves very similar to a function.
  • % like %re - The same as %%

Sounds good. Then let's highlight %%name and %name as functions.

@RunDevelopment

Copy link
Copy Markdown
Member

The coverage CI test is currently failing because:

  1. The token attr-value is completely untested. Please add a test for this.
  2. Not all keywords are tested. Please add a keyword test. Example.

Comment thread components/prism-rescript.js Outdated
@vmarcosp

Copy link
Copy Markdown
Contributor Author

The coverage CI test is currently failing because:

  1. The token attr-value is completely untested. Please add a test for this.
  2. Not all keywords are tested. Please add a keyword test. Example.

I'm running the tests locally just to make sure that everything is passing

@RunDevelopment

Copy link
Copy Markdown
Member

I'm running the tests locally just to make sure that everything is passing

Some parts of the CI aren't part of npm test. Coverage (npm run regex-coverage) is one of them.

Comment thread components/prism-rescript.js Outdated
Comment thread tests/languages/rescript/keyword_feature.test Outdated

@RunDevelopment RunDevelopment left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, @vmarcosp! I think the language is ready to go. Please run npm run build to make the CI and then I'll merge this.

@vmarcosp

Copy link
Copy Markdown
Contributor Author

Thank you, @vmarcosp! I think the language is ready to go. Please run npm run build to make the CI and then I'll merge this.

Thanks for your help 😄

@RunDevelopment
RunDevelopment merged commit cbef9af into PrismJS:master Apr 15, 2022
@vmarcosp

Copy link
Copy Markdown
Contributor Author

@RunDevelopment is there an estimate for the next release that includes these changes?

@RunDevelopment

Copy link
Copy Markdown
Member

Probably this weekend.

@vmarcosp

Copy link
Copy Markdown
Contributor Author

Oh, great! Tks

@vmarcosp

Copy link
Copy Markdown
Contributor Author

I almost forgot, but since this pr is merged I think you can close this issue #3094

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants