-
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
jsstacktrace: add language #2418
jsstacktrace: add language #2418
Conversation
Seems interesting. JS stack traces are 100% implementation-defined, so this might not be 100% perfect but why not? Before we can merge this:
Also, maybe add support for deno as well? Right now it's very centered on node. Examples stack traces from deno
If you need help in any way, feel free to ask. The Java stack trace implementation should be a good example. |
Thanks for the feedback! I'll review those comments and make some improvements. |
Done, @RunDevelopment - all suggestions applied, if you could take another look? |
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 a lot better!
I have a few suggestions. These should also fix most (if not all) of the highlighting errors in the examples page (it's really good that you included so many and so diverse examples!).
Also, there is an s
missing in the name of the examples file.
Also also, it would be nice to highlight (
and )
as punctuation.
Ah, thanks for all those comments @RunDevelopment! I've made some additional changes. Some notes:
Many great suggestions, thanks again! |
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.
@sbrl Looking good!
Just a few minor things and then we can merge this.
(I think this is going to be the last review round.)
Co-authored-by: Michael Schmidt <[email protected]>
Co-authored-by: Michael Schmidt <[email protected]>
… into feature/language-jsstacktrace
Applied those changes, @RunDevelopment. Is this good to go 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.
LGTM! Thank you for your work!
You have to rebuild to make the build pass and then we can merge this.
Phew! Glad to finally have this approved. Thanks for the help in the review process! I've rebuilt it and pushed a new commit . |
Thank you for contributing @sbrl! |
This language highlights JavaScript stack traces generated by commonly used JS engines.
I was getting really frustrated this afternoon with not being able to read long Javascript stack traces easily, so I implemented a simple grammar for Prism :-)