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

Add rego support #2624

Merged
merged 10 commits into from
Mar 7, 2021
Merged

Add rego support #2624

merged 10 commits into from
Mar 7, 2021

Conversation

JordanSh
Copy link
Contributor

@JordanSh JordanSh commented Nov 8, 2020

solving #2571

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 @JordanSh!

You seem to have implemented a subset of Rego. I.e. several keywords and features like raw strings are missing. Unfortunately, this means that the language definition will highlight a lot of relatively easy Rego code incorrectly. It also has a few errors (e.g. package and some are keywords and strings that contain a # will be highlighted incorrectly.)

I know that creating Prism language definitions isn't easy, so I want to ask whether you want to add all the missing features and fix the errors yourself or should I do it? Rego has excellent documentation, so I can make a language definition for it rather quickly. So would you like to make you a PR to your branch with all the fixes and additions?

@JordanSh
Copy link
Contributor Author

JordanSh commented Nov 11, 2020

i will try to handle this asap, in case i'll get stuck i'll let you know

@JordanSh
Copy link
Contributor Author

JordanSh commented Mar 4, 2021

Sorry for the late notice @RunDevelopment. I would like to request for the help you offered to finish this PR. let me know if that would be possible and if I can provide help of any kind.
Thanks!

@RunDevelopment
Copy link
Member

I would like to request for the help you offered to finish this PR

Sure thing! I will directly commit to this branch if that's alright with you.

let me know [...] if I can provide help of any kind.

If possible, I would like you to test/review my changes. I can implement syntax highlighting from skimming through the documentation but having someone with in-depth knowledge of the language review it is very valuable.

@JordanSh
Copy link
Contributor Author

JordanSh commented Mar 4, 2021

Sounds perfect! :)

The full example contains both comments and strings.
Rego is a rather complex language with a syntax that is not easy to
highlight. I implemented the minimum amount of highlighting that can be
done with making any assumptions/approximations. It should be correct
but it might not be very useful to actual Rego users.
@github-actions
Copy link

github-actions bot commented Mar 5, 2021

JS File Size Changes (gzipped)

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

file master pull size diff % diff
components/prism-rego.min.js 0 Bytes 365 B +365 B +100.0%

Generated by 🚫 dangerJS against a830a7e

@RunDevelopment
Copy link
Member

@JordanSh I made my changes.

I only implemented minimal language support. All language features are supported but not all get special highlighting. Please review and tell me whether the highlighting makes sense.

I removed the highlighting for constants and the input variable because they aren't part of the language grammar. It might be beneficial to re-add them I have no experience with Rego, so I can't make that judgment.

@JordanSh
Copy link
Contributor Author

JordanSh commented Mar 7, 2021

@RunDevelopment, i played with the changes and it looks great!

I removed the highlighting for constants and the input variable because they aren't part of the language grammar. It might be beneficial to re-add them I have no experience with Rego, so I can't make that judgment.

All and all I believe that this is the right decision. input has specific use cases which are really common at a certain context, especially the one we are using, but it is indeed not an integrated part of Rego itself.

@RunDevelopment
Copy link
Member

Alright, since you accept, I think this is ready.

@RunDevelopment RunDevelopment merged commit e38986f into PrismJS:master Mar 7, 2021
@ralgozino
Copy link

Thank you @JordanSh for taking care of this :) <3

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.

3 participants