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 Java Stack Trace highlight support. #916

Closed
wants to merge 1 commit into from

Conversation

wesalvaro
Copy link

Highlights Java stack traces.

@wesalvaro wesalvaro force-pushed the stack-traces branch 3 times, most recently from 3403875 to 0031c40 Compare March 25, 2016 06:56
@zeitgeist87
Copy link
Collaborator

Hi @wesalvaro

Thanks for contributing. I like your idea. This could be quite useful.

I'll try to do a proper review until tomorrow.

@wesalvaro
Copy link
Author

Sounds great. I look forward to hearing your suggestions.

@zeitgeist87
Copy link
Collaborator

Your solution is very precise. It is almost a complete semantically correct parser for Java stack traces. It must have taken you a lot of effort to get it there.

However I think in its current version it is unnecessarily complex. Prism is not a parser and it strives to be as simple and fast as possible. I can get almost the same result with the following simple language definition:

Prism.languages.stackjava = {
    'function': /[a-z0-9_$<>-]+(?=\()/i,
    'path': {
        pattern: /[a-z0-9_]+\.java(?=:\d+)/i,
        alias: 'entity',
    },
    'package': {
        pattern: /\b[a-z0-9_]+(?:\.[a-z0-9_$]+)+/i,
        inside: {
            'class-name': {
                pattern: /(\.)[^.]+$/,
                lookbehind: true,
                alias: 'entity'
            }
        }
    },
    'keyword': /\b(?:Caused by|at|more)\b/,
    'number': /\d+/,
    'punctuation': /[:.()]/,
};

The above definition exploits the fact that Prism executes every pattern in order and that later patterns cannot overwrite existing matches. The above definition is of course just a quick draft to show you what I mean. It is not tested very well and only intended as a rough guide.

I like how you constructed the IDENTIFIER_CLS variable.

Another minor thing Prism generally uses tabs instead of spaces.

I hope you are not discouraged by my critique. I think adding a language for Java stack traces is a good idea.

EDIT:

On second thought, do we really need all those obscure unicode characters in JAVA_IDENTIFIERS? I know they are theoretically possible, but I have never seen any of them used in practice. Prism doesn't have to be a compliant parser.

@wesalvaro
Copy link
Author

Thanks for the review! I admit that I struggled a bit getting to the point where things were recognized in a coherent way. Our project also includes a plugin to link classes and things in the stack traces to the actual code files, so that's part of the rationale for detecting things the way I did. However, I'm sure it's a little overboard.

I will try out your suggestions and see how they go with the test cases that I amassed from our stack trace investigations.

As for the JAVA_IDENTIFIERS, we were seeing some weird identifiers in the stack traces and so I felt that completeness there was a fine approach. I would say it's definitely (hopefully) unnecessary, though.

Glad to know that you like the idea. I'll work on it some more this week and update you on the result.

Thanks again,
-Wes

@wesalvaro wesalvaro force-pushed the stack-traces branch 2 times, most recently from a6ababc to 7176690 Compare May 30, 2016 08:44
Highlights Java stack traces.
@mAAdhaTTah
Copy link
Member

@Golmote You think this would be useful to include?

@Golmote
Copy link
Contributor

Golmote commented May 27, 2018

@mAAdhaTTah I'm ok with it. I wasn't sure it was a finished PR, since @wesalvaro never commented after his last commit, and some on the stuff @zeitgeist87 pointed out have not been addressed at all (like usings tabs).
An example file would also be needed, and maybe more atomic tests.

I don't think we have write access on this one, so we'll need to make a new PR if @wesalvaro does not update it himself.

@mAAdhaTTah
Copy link
Member

Superceded by #1520.

@mAAdhaTTah mAAdhaTTah closed this Dec 1, 2018
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.

4 participants