-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added support for Arturo #3403
Conversation
JS File Size Changes (gzipped)A total of 3 files have changed, with a combined diff of +1.62 KB (+32.1%).
|
There was a problem hiding this 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).
@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%! 😉 |
I did both 😉 In the case of punctuation, I just removed In the case of color, I think your regex will work fine. Actually the rules are quite complicated. What is recognized as a
So, as a better approach without over-complicating things, I think your suggestion is fine. :) |
There was a problem hiding this 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.
…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
resolve conflicts
I think I made it (I hope I didn't manage to break everything - Git is not my strongest point lol 🙄 ) |
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! 😉 |
➕ Support for the Arturo programming language