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 Arturo #3403

Merged
merged 73 commits into from Mar 21, 2022
Merged

Added support for Arturo #3403

merged 73 commits into from Mar 21, 2022

Conversation

drkameleon
Copy link
Contributor

➕ Support for the Arturo programming language

Screenshot 2022-03-19 at 12 31 06

@github-actions
Copy link

github-actions bot commented Mar 19, 2022

JS File Size Changes (gzipped)

A total of 3 files have changed, with a combined diff of +1.62 KB (+32.1%).

file master pull size diff % diff
components/prism-arturo.min.js 0 Bytes 1.61 KB +1.61 KB +100.0%
plugins/autoloader/prism-autoloader.min.js 2.39 KB 2.39 KB +7 B +0.3%
plugins/show-language/prism-show-language.min.js 2.66 KB 2.67 KB +6 B +0.2%

Generated by 🚫 dangerJS against 85b9e31

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

I gave your PR a quick look and left you some comments. I'll re-review it again after the CI passes.

The CI currently fails because of the linter and coverage tests.

Just run npm run lint:fix to fix all the indentation problems. Our ESLint config contains some very computationally intensive rules, so this might take a minute or two (yes, really). The error with the symbol regex is a bug, however. Please see my comment on this one.

To appease the coverage test, you just has to add a few more tests. To see what regexes still need tests, you can see the CI logs or run npm run regex-coverage locally. The coverage test requires that all keywords are tested, so please add all of them to the test files. You don't need a valid code snippet for each keyword. It's fine to just have them all as a list (example).

examples/prism-arturo.html Outdated Show resolved Hide resolved
components/prism-arturo.js Outdated Show resolved Hide resolved
components/prism-arturo.js Outdated Show resolved Hide resolved
@drkameleon
Copy link
Contributor Author

drkameleon commented Mar 20, 2022

@RunDevelopment Apologies for the abrupt break but... since it's weekend, I decided to take some time off. It's been quite a killer week.

I'll get back to working on the PR by tomorrow morning so that we can finally complete it; cleaning up the code/tests a bit and taking into account ofc all of your super-valuable suggestions.

Thanks a lot - I appreciate your assistance more than 100%! 😉

@drkameleon
Copy link
Contributor Author

drkameleon commented Mar 21, 2022

Could we please also add support for punctuation? The theme you use doesn't highlight punctuation but many others do, so it would be nice to include it.

'punctuation': /[()[\]:.,#]/

I probably missed some in the regex.

Could we also add a pattern for colors? We have a plugin that displays colors inline, so it would be nice if it could support Arturo.

'color': /#\w+/

(I'm not too sure about the specific syntax of colors, so I just went with "any word". I hope that's okay.)

I did both 😉

In the case of punctuation, I just removed # and ., since they are use for different things (the first one as a symbol, alias to the function dictionary and the second one is syntax for "attributes")

In the case of color, I think your regex will work fine. Actually the rules are quite complicated. What is recognized as a :color value:

  • 3-digit RGB hex (e.g. #F00)
  • 6-digit RGB hex (e.g. #FF0000)
  • 8-digit RGBA hex (e.g. #FF000088)
  • custom W3C-compliant (lower)-camelCase color names (e.g. #red or #darkOliveGreen)

So, as a better approach without over-complicating things, I think your suggestion is fine. :)

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.

Final round. I think that language definition is pretty much gtg right now.

Please fix the merge conflict. This will be a little tricky in components.json (because you have to do it manually), but after you resolved that file, you can just run npm run build and all other conflicts will be resolved automagically.

tests/languages/arturo!+bash/sh-string_feature.test Outdated Show resolved Hide resolved
drkameleon and others added 5 commits March 21, 2022 16:16
…S-master

# Conflicts:
#	components.js
#	plugins/autoloader/prism-autoloader.js
#	plugins/autoloader/prism-autoloader.min.js
#	plugins/show-language/prism-show-language.js
#	plugins/show-language/prism-show-language.min.js
@drkameleon
Copy link
Contributor Author

Final round. I think that language definition is pretty much gtg right now.

Please fix the merge conflict. This will be a little tricky in components.json (because you have to do it manually), but after you resolved that file, you can just run npm run build and all other conflicts will be resolved automagically.

I think I made it (I hope I didn't manage to break everything - Git is not my strongest point lol 🙄 )

@RunDevelopment RunDevelopment merged commit e2fe1f7 into PrismJS:master Mar 21, 2022
@RunDevelopment
Copy link
Member

Thank you for contributing @drkameleon!

@drkameleon
Copy link
Contributor Author

Thank you for contributing @drkameleon!

Thank you for the extremely helpful input and all the assistance!

You are a top coder - and I would be more than happy to see you contribute to Arturo too (in case you have time and wish, that is)

Have a great day! 😉

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.

None yet

2 participants