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 support for Jexl #2764

Merged
merged 2 commits into from
Mar 21, 2021
Merged

Add support for Jexl #2764

merged 2 commits into from
Mar 21, 2021

Conversation

czosel
Copy link
Contributor

@czosel czosel commented Feb 14, 2021

@github-actions
Copy link

github-actions bot commented Feb 14, 2021

JS File Size Changes (gzipped)

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

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

Generated by 🚫 dangerJS against 318817a

@czosel czosel force-pushed the add-jexl branch 3 times, most recently from 7860844 to bbe679c Compare February 14, 2021 14:30
@czosel czosel changed the title Add support for Jexl WIP: Add support for Jexl Feb 14, 2021
@czosel czosel changed the title WIP: Add support for Jexl Add support for Jexl Feb 14, 2021
@czosel
Copy link
Contributor Author

czosel commented Feb 20, 2021

@RunDevelopment Before I resolve the conflicts, is this something you‘d be interested in? Thanks in advance!

@czosel
Copy link
Contributor Author

czosel commented Feb 27, 2021

Conflicts are resolved.

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.

Sorry for the delay @czosel.

I looked at Jexl and your language definition and there are few things that need to be addressed (nothing major).

Also, please follow the quoting style we use. We use single quotes and token names are always quoted. See CSS as an example.


I want to ask about the current status of the Jexl project. There hasn't been any activity for half a year now which is kinda worrying.

components/prism-jexl.js Outdated Show resolved Hide resolved
components/prism-jexl.js Outdated Show resolved Hide resolved
components/prism-jexl.js Outdated Show resolved Hide resolved
components/prism-jexl.js Outdated Show resolved Hide resolved
tests/languages/jexl/operator_feature.test Outdated Show resolved Hide resolved
@czosel
Copy link
Contributor Author

czosel commented Mar 8, 2021

Hi @RunDevelopment, thanks for the detailed review! I'll address the points you mention over the coming days.

Regarding the status of the project: It's a small language (just expressions), I'd see the fact that nothing happened over the last 6 months as a sign of stability 😉 I'm not involved with the Jexl project myself, but my company is a heavy user of Jexl and would potentially be committed to help maintaining it, should the need ever arise.

@RunDevelopment
Copy link
Member

I'd see the fact that nothing happened over the last 6 months as a sign of stability

The lack of commits, yes. What worried me more was that nobody responds to new issues and PRs.

But it's good to know that there are people willing to step in.

@czosel czosel force-pushed the add-jexl branch 2 times, most recently from 8f00058 to 72a511b Compare March 21, 2021 14:41
@czosel
Copy link
Contributor Author

czosel commented Mar 21, 2021

Hi @RunDevelopment
Sorry this took a bit longer than expected. I was able to address most of your feedback - thanks for taking another look!

components/prism-jexl.js Outdated Show resolved Hide resolved
components/prism-jexl.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

Please ignore the last review comment. The "It really isn't. I'm sorry, this is my bad. I assumed that .4 (as in JS) is a number. In that case, both the original and my suggested pattern are incorrect. I'll suggest a new one." one. GitHub has a bug with its review comment system and I triggered that bug...

@czosel
Copy link
Contributor Author

czosel commented Mar 21, 2021

@RunDevelopment Should we still change something about the numbers?

@RunDevelopment
Copy link
Member

I say we just leave it as is.

@RunDevelopment RunDevelopment merged commit 7e51b99 into PrismJS:master Mar 21, 2021
@RunDevelopment
Copy link
Member

Thank you for contributing @czosel!

@czosel
Copy link
Contributor Author

czosel commented Mar 21, 2021

Awesome 🎉 Thanks for the detailed review, and your work on Prism in general!

Do you already have a new release planned? :-)

@RunDevelopment
Copy link
Member

Thanks.

I do want to make a new release soon-ish but not until a few of the more recent PRs get merged.

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.

2 participants