-
Notifications
You must be signed in to change notification settings - Fork 520
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
Update stacktrace to include trimmed? and use color? in web #387
Conversation
Thanks for the commits. Can you just upload some screenshots of before and after so there's a record of what you're doing? It's hard to tell from the description alone. Can you also make sure the PR adheres to the contributing guidelines, particularly the parts around commit message, changes unrelated to the purpose of the PR, and line length. |
This comment has been minimized.
This comment has been minimized.
The new parameter will display the key :trimmed-elems returned from the processes error instead of :trace-elems which whill show a shorter trace. This is benificial in 1. not flooding the terminal and 2. making it easier to find the trace directly related to the application. Extending color? to wrap-stacktrace-web will allow the same color patterns available in wrap-stacktrace-log.
Can you just show part of the screen? It would help to have a record of what it looks like in terms of before and after. |
Also in terms of review, please see the contributing guidelines first. There's a bunch of formatting changes that need to be taken out, and the commit messages need to adhere to the guidelines. |
I read the guidelines and did as best as I can interpret on the commit messages, and they are updated from the initial push. You have to realize that I have never had a project with those types of guidelines, it's not obvious to me what you are asking me to change. As far as the code, I dont see any formatting changes in the PR. Please post a review on github, I can make any changes you ask for. |
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.
I've highlighted the formatting changes to remove; a PR should implement a single change or feature. There are also some code style issues, and some functions that are public but no explanation of why they're public. Additions to the public API of Ring need to be justified.
Thank you for the review that helped me understand what you are looking for. I found a couple more formatting changes to remove after making the updates you pointed out. Sorry, I didnt see the clojure guide also had a line length. Not fixing indentation on the code I'm working on is new to me. Should I create a PR after this one to fix the indents, and other formatting? |
Another PR to fix indentation would be fine. The reason we don't mix formatting changes with feature changes is that it muddies the diff and makes it hard to understand what has changed. If we separate out formatting changes from other changes, it's a lot easier to see which lines have changed. This code looks like it could be written more concisely; there are a lot of functions where I think a refactor would significantly reduce the changes required. I've also briefly looked into |
You are correct that the root does not have trimmed elements, only causes do. The root |
Would it be better if we just made certain segments of the stacktrace collapsable? I'm also wondering if we want to test this out in an external library first, as I currently lack the time to investigate this thoroughy. |
I could do this, instead of 1 new option, it would be two: collapse-root? and trimmed-cause? The code would get more complex though. As written works for me, until someone requests some parts collapsible and other not, it feels like extra. I have the code running in my current project from a local NS. How would I get it into an external library for testing? |
You can just pull the namespace out, give it a new name, and release it as part of a new library. That will solve the problem in the short term. In the longer term I'll get around to looking at this eventually, but I'll need to budget several days review. Currently I don't feel as though I understand the consequences of this change well enough to even begin to think about merging it, and I'm pretty sure we could make the code more concise in places. |
Thanks for all your feedback. To help the review process, I can break out the changes for formatting, color? and trimmed?. |
This PR will be split, and is no longer needed. |
I expanded the use of the color? option to show colors on the html response for (wrap-stacktrace-web ) based on the error type (java, clojure, etc). This uses the same logic for determining color as (wrap-stacktrace-log).
I also added a new tag trimmed? to use the :trimmed-elems instead of :trace-elems. :trimmed-elems is a shorter stack trace provided the error report. You can see an example in current code by comparing the log output and the web output for a :cause. The log output already uses :trimmed-elems in pst-on.
closes #386