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

components: add hoon programming language #2978

Merged
merged 15 commits into from Jul 3, 2021
Merged

Conversation

@matildepark
Copy link
Contributor

@matildepark matildepark commented Jul 2, 2021

We're using this downstream on https://urbit.org (in a site using next.js and remark-prism), so passing this language definition upstream. Derived from our tmLanguage definition and the documentation.

@github-actions
Copy link

@github-actions github-actions bot commented Jul 2, 2021

JS File Size Changes (gzipped)

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

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

Generated by 🚫 dangerJS against 112b95d

Loading

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Thank you for the PR @matildepark!

Please fix the linting error and make the tests pass.

One reason that the tests don't pass is that you used capturing groups extensively. Except in the case of lookbehinds, you can replace all capturing groups with non-capturing groups. This should fix most of the problems our tests find.

Also, please add language tests for Hoon. We regularly improve our matching algorithm and tooling. Language tests are essential to make sure that nothing breaks. Please add tests that cover every regex in Hoon.
Step 6 in this guide explains how to add tests. For a more detailed explanation, see the test suite documentation.

If you run into problems or have any questions, feel free to ask anytime. I'm here to help.

Loading

components.json Outdated Show resolved Hide resolved
Loading
components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
@matildepark matildepark requested a review from RunDevelopment Jul 2, 2021
@matildepark
Copy link
Contributor Author

@matildepark matildepark commented Jul 2, 2021

Should resolve, I think — @RunDevelopment

Loading

Copy link
Member

@RunDevelopment RunDevelopment left a comment

I have a few minor suggestions and I think I found a bug.

Loading

components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
matildepark and others added 5 commits Jul 2, 2021
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
@matildepark
Copy link
Contributor Author

@matildepark matildepark commented Jul 2, 2021

I believe everything addressed again; it's failing a test on identifiers, which I'm a little confused about; you can't name something foo_123 in hoon; it's generally kabob case, and _ is used in runic constructions (or our 'keywords'). Should Hoon be excluded from that? Also added the same example of the cipher, as I think it includes most of the features you'd highlight in one example.

Loading

@matildepark matildepark requested a review from RunDevelopment Jul 2, 2021
@matildepark
Copy link
Contributor Author

@matildepark matildepark commented Jul 2, 2021

Oh, right, I also reverted the change in 3982e75 because it was a broken definition.

Loading

@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 2, 2021

Oh, right, I also reverted the change in 3982e75 because it was a broken definition.

Indeed. I'll make a new suggestion.

Loading

components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 2, 2021

it's failing a test on identifiers, which I'm a little confused about; you can't name something foo_123 in hoon; it's generally kabob case, and _ is used in runic constructions (or our 'keywords'). Should Hoon be excluded from that?

Yeah, just exclude it here. I think you only need to disable word and template.

Loading

components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
@matildepark
Copy link
Contributor Author

@matildepark matildepark commented Jul 2, 2021

Oh, lol, it's because I changed it to function-name. OK, one sec ...

Loading

@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 2, 2021

You can let the test runner update all tests with npm run test:languages -- --update.

Loading

@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 2, 2021

Nice, tests pass. Just an npm run build and this is ready.

Loading

@matildepark
Copy link
Contributor Author

@matildepark matildepark commented Jul 2, 2021

Built!

Loading

@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 2, 2021

Thank you @matildepark!

It's ready as is but we are currently fixing 2 bugs that came up after the last release. I'll merge this PR right after the patch for these 2 bugs is released, maybe in 1 or 2 days.

Loading

@matildepark
Copy link
Contributor Author

@matildepark matildepark commented Jul 2, 2021

No worries, thanks for the patience.

Loading

components/prism-hoon.js Outdated Show resolved Hide resolved
Loading
@matildepark matildepark requested a review from RunDevelopment Jul 3, 2021
@RunDevelopment RunDevelopment merged commit ea77675 into PrismJS:master Jul 3, 2021
11 checks passed
Loading
@RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jul 3, 2021

Thank you for contributing @matildepark!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants