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 TypoScript #2505

Merged
merged 18 commits into from
Aug 11, 2020
Merged

Added support for TypoScript #2505

merged 18 commits into from
Aug 11, 2020

Conversation

dkern
Copy link
Contributor

@dkern dkern commented Aug 5, 2020

Added support for TypoScript/TSConfig highlighting.

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 language definition @dkern!

I left you a few comments. Also:

  1. None of the regex literals need a g flag. If Prism needs it, it will add it.
  2. We use tabs for indentation.
  3. Please add some tests.

components.json Show resolved Hide resolved
components/prism-typoscript.js Outdated Show resolved Hide resolved
components/prism-typoscript.js Outdated Show resolved Hide resolved
components/prism-typoscript.js Outdated Show resolved Hide resolved
components/prism-typoscript.js Outdated Show resolved Hide resolved
@dkern
Copy link
Contributor Author

dkern commented Aug 10, 2020

1. None of the regex literals need a `g` flag. If Prism needs it, it will add it.

Will be fixed! ;)

2. We use tabs for indentation.

Will check this too.

3. Please add some [tests](https://prismjs.com/test-suite.html).

I take a short look at it, but to be honest, i already spend way to much time on this language creation. 😉

@dkern
Copy link
Contributor Author

dkern commented Aug 10, 2020

Two further questions:

  1. Are comments allowed, explaining some of the regex in the non-minified file?

  2. Is there a way to use internal reverence, or is it possible to declare a variable somewhere to store something? I need the keywords matching regex on two places. To save data, i could use the same regex for both. Is there any way or should i just insert them two times?

@RunDevelopment
Copy link
Member

Are comments allowed

Of course!

Is there a way to use internal reference, or is it possible to declare a variable somewhere to store something? I need the keywords matching regex on two places. To save data, i could use the same regex for both. Is there any way or should i just insert them two times?

Here is how the Java language definition does it. Please note that the IIFE is necessary to prevent namespace pollution.

@dkern
Copy link
Contributor Author

dkern commented Aug 10, 2020

Thank you for your reply. I'm now completely finished with my work, but the default tests fails, because of one regex group. But i have no idea how to fix this. I need a bit of help here.

The Problem is my comment matching. General a comment always starts with //. But if there is a leading : or \\ it is not a comment, or whenever there is = with optional space directly in front, it's not a comment too. I have the regex working fine, but the prism tests fails, because there is a matching group. But i can't make the group non-matching with ?:, because of the lookbehind.

My regex:

{
	// double-slash comments - ignored when backslashes or colon is found in front
	// also ignored whenever directly after an equal-sign, because it would probably be an url without protocol
	pattern: /(^|(?<!\\|:|=\s*))\/\/.*/,
	lookbehind: true,
	greedy: true
},

Here is a working example of it. Everything is fine, but i would not pass the test:
https://regex101.com/r/RYh0Og/1/

Would be nice if someone has a hint how to do this. Thanks!

@RunDevelopment
Copy link
Member

The problem in /(^|(?<!\\|:|=\s*))\/\/.*/ is the ES2018 lookbehind (?<!\\|:|=\s*). ES2018 lookbehinds are fairly new, so we can't use them. That's why we have these pseudo-lookbehind groups via a capturing group.

But the problem with Prism lookbehind groups is that we can't easily negate them. This is not trivial, so here's your pattern with the ES2018 lookbehind: /(^|[^\\:=\s]|(?:^|[^=\s])\s+)\/\/.*/

However, your tests still don't pass because with the second comment here not being highlighted:

none match text // a comment
// a comment

Why is the second comment not highlighted? The problem is that after the first comment, the next character is a \n. \s matches \n, so we need to find a character that matches [^\s=] before it. But we cannot find such a character because the regex engine started matching at \n and cannot go back (without ES2018 lookbehinds) to the t of the previous comment. Thus the second comment cannot be matched.

How do we fix that?
We can't... We can only change the pattern. If we use the pseudo-lookbehind of (?<!\\|:|=[ \t]*) instead, it will work. This means that code like the following to be a false positive but I hope that's acceptable.

=
// false positive

Final pattern: /(^|[^\\:= \t]|(?:^|[^= \t])[ \t]+)\/\/.*/

@dkern
Copy link
Contributor Author

dkern commented Aug 10, 2020

Thank you @RunDevelopment ! I've tested your pattern and it works perfectly! Such things are way above my regex-knowledge. ;)

So, a second before your comment, I've pushed my changes. I've completely redone my language definition and managed to removed the whole tag list, so it is now more a syntax based highlighting. I'm pretty pleased with the outcome. The highlighting is much better then in my first submission. Overall, i have noting left to do. Everything is covered so far.

I think i have managed to get all your things in the review covered. I've added some tests too and hope that everything is ok 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.

Looks good!

I still have some minor nits but it's basically ready.

You do need to resolve the merge conflicts. All of the conflicts are with generated files, so after git merge, running npm run build should resolve all of them.

components/prism-typoscript.js Outdated Show resolved Hide resolved
components/prism-typoscript.js Outdated Show resolved Hide resolved
components/prism-typoscript.js Outdated Show resolved Hide resolved
docs/Prism.html Outdated Show resolved Hide resolved
# Conflicts:
#	components.js
#	plugins/autoloader/prism-autoloader.min.js
#	plugins/show-language/prism-show-language.min.js
@dkern
Copy link
Contributor Author

dkern commented Aug 11, 2020

I think I've resolved everything so far. Hopefully we're fine now. 😉

@RunDevelopment
Copy link
Member

@dkern The last remaining issue is the comment pattern. The easiest solution might be to copy it from clike.

@dkern
Copy link
Contributor Author

dkern commented Aug 11, 2020

@dkern The last remaining issue is the comment pattern. The easiest solution might be to copy it from clike.

Done.

@RunDevelopment RunDevelopment merged commit bf115f4 into PrismJS:master Aug 11, 2020
@RunDevelopment
Copy link
Member

Thank you for contributing @dkern!

quentinvernot pushed a commit to TankerHQ/prismjs that referenced this pull request Sep 11, 2020
Copy link

@kronos361 kronos361 left a comment

Choose a reason for hiding this comment

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

Excellent job

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

3 participants