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

Added support for ReScript #3435

Merged
merged 20 commits into from
Apr 15, 2022

Conversation

vmarcosp
Copy link
Contributor

@vmarcosp vmarcosp commented Apr 15, 2022

Added support for ReScript

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

@github-actions
Copy link

github-actions bot commented Apr 15, 2022

JS File Size Changes (gzipped)

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

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

Copy link

@fdaciuk fdaciuk left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@RunDevelopment RunDevelopment left a comment

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.

components.json Outdated Show resolved Hide resolved
components/prism-rescript.js Show resolved Hide resolved
components/prism-rescript.js Outdated Show resolved Hide resolved
components/prism-rescript.js Outdated Show resolved Hide resolved
components/prism-rescript.js Outdated Show resolved Hide resolved
components/prism-rescript.js Outdated Show resolved Hide resolved
@vmarcosp
Copy link
Contributor Author

vmarcosp commented Apr 15, 2022

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

@fakenickels
Copy link

🔥🔥🔥

@RunDevelopment
Copy link
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.

@RunDevelopment
Copy link
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.

@vmarcosp
Copy link
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.

@RunDevelopment
Copy link
Member

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

@RunDevelopment
Copy link
Member

I have a question about regexes. Is %re("/b/g") special syntax, or is %re just a weird function name?

@vmarcosp
Copy link
Contributor Author

I have a question about regexes. Is %re("/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
Member

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

@vmarcosp
Copy link
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 %%

@RunDevelopment
Copy link
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
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.

@vmarcosp
Copy link
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
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.

Copy link
Member

@RunDevelopment RunDevelopment left a comment

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
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
Contributor Author

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

@RunDevelopment
Copy link
Member

Probably this weekend.

@vmarcosp
Copy link
Contributor Author

Oh, great! Tks

@vmarcosp
Copy link
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