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 Clojure #1311

Merged
merged 24 commits into from
Mar 2, 2018
Merged

Add Clojure #1311

merged 24 commits into from
Mar 2, 2018

Conversation

athomasoriginal
Copy link
Contributor

@athomasoriginal athomasoriginal commented Feb 28, 2018

Closes #1147

Implements the updates requested in #1214 and adds syntax highlighting for:

  • thread-first (->) macro
  • thread-last (->>) macro
  • nil

Regarding the updates, you will notice that each of my commits addresses the comments made by @Golmote in #1214. Further, each commit should be in the order of the requested fixes. I apologize for not piggybacking directly off the other PR.

Let me know if there is anything else I can improve upon.

@athomasoriginal athomasoriginal mentioned this pull request Feb 28, 2018
Forgot to compile with gulp in the last commit.  my bad.
@paladox paladox mentioned this pull request Mar 1, 2018
@paladox
Copy link
Contributor

paladox commented Mar 1, 2018

cc @Golmote wondering if you could review this please? :)

Copy link
Contributor

@Golmote Golmote left a comment

Choose a reason for hiding this comment

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

@tkjone Hi! Thanks for taking care of this. I added just a few comments. We're almost there!

Prism.languages.clojure = {
comment: /;+.*/,
string: /"(?:\\.|[^\\"\r\n])*"/,
operator: /(::|[:|'])\b[a-zA-Z][a-zA-Z0-9*+!-_?]*\b/g, //used for symbols and keywords
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the group non-capturing here? And also remove the g flag. You could add the i flag to shorten the regexp a bit, and use \w in the second character class. Finally, I think the last - is supposed to be escaped (or moved at the end).
The whole pattern could be something like:
/(?:::|[:|'])\b[a-z][\w*+!?-]*\b/i

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

@@ -0,0 +1,13 @@
false
true

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test for nil here? Since it's handled in the boolean pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

(let)
(var)
(fn)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we should add checks for each and every keyword here.

Copy link
Contributor Author

@athomasoriginal athomasoriginal Mar 2, 2018

Choose a reason for hiding this comment

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

I added about 70 new keywords here - a lot of the ones that were posing issues when I was testing e.g. <= and >=. I am happy to fill out the rest, but I have a high degree of confidence that the ones I did not include will pass 😉


[
["punctuation", "("],
["keyword", "def"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you indent those lines with a tab, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finished 😌

@athomasoriginal
Copy link
Contributor Author

@Golmote Thanks for the review! Let me know what you think 💪

@Golmote
Copy link
Contributor

Golmote commented Mar 2, 2018

@tkjone Yay! LGTM!

@Golmote Golmote merged commit 8b4d3bd into PrismJS:gh-pages Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants