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 NaniScript #2494

Merged
merged 25 commits into from
Aug 7, 2020
Merged

Add support for NaniScript #2494

merged 25 commits into from
Aug 7, 2020

Conversation

elringus
Copy link
Contributor

@elringus elringus commented Aug 4, 2020

NaniScript is a scenario scripting language used by Naninovel visual novel extension for Unity: https://naninovel.com

@elringus
Copy link
Contributor Author

elringus commented Aug 5, 2020

Sorry for the ongoing commits; it's ready now.

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 making this pull request @elringus!

There are a few things we have to do before we can merge this.

I think it's largely ok as is (I looked at your documentation page and the highlight looks really good) but there a few smaller things we have to do (mostly simplifying and optimizing the regexes).

Also, you don't have to do these alone. If it's ok with you, then I'll make a PR to your branch that will include some of my suggestions.

components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
@elringus
Copy link
Contributor Author

elringus commented Aug 6, 2020

Thank you for the review! @vladdancer is actually the main contributor here, hopefully he'll be able to provide some feedback on the suggestions. A PR to the fork would be great, thanks!

@RunDevelopment
Copy link
Member

Quick question: How does escaping in strings work? The doc gives this example:

"Saying \\"Stop the car\\" was a mistake."

But I guess that this isn't how it is supposed to be tokenized:

image
(Screenshot of the DOM of the website)

So how does it work? It's not mentioned anywhere and the double escaping seems weird to me.

@elringus
Copy link
Contributor Author

elringus commented Aug 6, 2020

That's a special case here. The value of if operators is treated as expression by default (to save the user from typing curly braces every time) and expressions are evaluated with their own parser after being parsed by the main one, hence the need for double escape: \\" is replaced with \" on the initial parse and then the expression evaluator receive a properly escaped quote.

@RunDevelopment
Copy link
Member

I see. Since doing this correctly will be quite difficult, let's just leave it as is.

@vladdancer
Copy link

@RunDevelopment thank you for a such great review!
I fixed remain issues and a couple of small improvements, added PR in Elringus repo.

Fix misc issues: new line, correct regex, right tests descriptions.
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 changes @vladdancer and @elringus!

I still have some minor nits but after those, it's ready to go.

components/prism-naniscript.js Outdated Show resolved Hide resolved
tests/languages/naniscript/label_feature.test Show resolved Hide resolved
components/prism-naniscript.js Outdated Show resolved Hide resolved
elringus and others added 3 commits August 7, 2020 21:00
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
@RunDevelopment RunDevelopment merged commit 388ad99 into PrismJS:master Aug 7, 2020
@RunDevelopment
Copy link
Member

Thank you for contributing @elringus and @vladdancer!

@elringus
Copy link
Contributor Author

elringus commented Aug 7, 2020

Thank you for the help and patience with us!

@vladdancer
Copy link

@RunDevelopment Thanks one more time for such great review, it gives me more details about regular expressions!

quentinvernot pushed a commit to TankerHQ/prismjs that referenced this pull request Sep 11, 2020
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.

3 participants