-
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
Improved javascript regex highlight #1468
Conversation
fa018c9
to
7271bf2
Compare
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.
Nice PR. I had something similar in mind, so I will share my own ideas as well.
-
I don't think that this alias-approach for styling is good. The fact that the alias
escape
doesn't do any coloring in the default themes, doesn't help.
(I personally prefer the subtle color changes for regexes like in editors like VS Code, but this requires that you change all themes Prism offers, so that might be a little overkill...) -
Please add highlighting for escapes and character ranges inside of character sets.
This is where I for example use escapes the most often.
Apart from these two things, I really like it. Keep up the good work ^^
6e43fda
to
5ddc3fd
Compare
Regarding the quantifiers: Regarding the escapes: |
I started to think that highlighting regex is not a simple job, maybe I should treat regex as a kind of "pseudo language" rather than just improve highlight for some situations. I'll keep trying. |
5ddc3fd
to
8793845
Compare
8793845
to
fbb7100
Compare
Hi, I have re-written my code according to the reference in https://regexr.com. In this reference, Now the code can recognize It's a pity that recognizing |
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.
Nice work!
With this, you now support almost the full JS regex syntax!
I do have some suggestions:
- Run
escape
before any other pattern matching backslashes.
This will ensure that you don't accidentally match something which is part of an escape sequence. Just add what you want to match later to the[^ ... ]
at the end. - Rename
regex-charset1
toregex-charset
and only let it capture stuff like[^...]
. Also renameregex-charset2
toregex-charclass
and let it capture.
,\d
can co.
This will make it easier to correctly match\d
(and co). Right now,\\d
is matched as acharset1
. Searching for it after the escapes ensures correctness. (This requires that the escapes don't match them.)
Apart from that and the minor things I commented on in the code, this looks pretty good.
Also, using VS Code and RegExr for reference was a good idea.
The only thing that is left to do now is to highlight the stuff that is inside of char sets and that's it.
Run Highlighting stuff inside the character set was not taken into consideration at first, but if it's necessary, using Other suggestions seems great, I'll make changes to my code tomorrow, it's midnight here now :( |
You are absolutely right. Don't worry, take your time ^^ |
I have pushed another commit, which fixes the bugs and provides more test cases. The most important thing is: it can recognize stuff inside charset now! |
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.
Looks really good! Well done!
Now, apart from the comments, I have some other things:
- You used the pattern for
escape
twice.
It's quite long. Maybe you could extract it into a variable? (Don't forget to wrap the whole thing with a function block. Just like e.g. Crystal.) - Maybe we could highlight the
[]
of a char set as something likepunctuation
? - Ranges are hard. Do we really need them?
(Look at my comment for more info)
All issues are fixed, |
Looks good! From a correctness point of view, this PR is ready for merge, but the maintainers might have a thing or two to say about it. |
Maybe this should be combined w/ #1714 and put into a |
Improve JS regex highlight according to references in https://regexr.com/.
Note: this PR can't recognize
capture group
because "matching bracket" is not a thing where a pure regex way can do.