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

Make backtrace cleaning optional #328

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

vala
Copy link

@vala vala commented Feb 26, 2016

This is a new take of the previous #242 pull request by @chancancode.

I borrowed the tests and adapted the implementation he wrote at the time to work with the current gem's code.

But, to make the tests pass, I had to add the ability to clean the backtracke to the #background_exception_notification since the EmailNotifier tests are written with the background notification.

This seems bad to me, since we wouldn't have the ability to disable backtrace cleaning only for the controller-exception backed email notifier.

Which way should I go ? Write a separate test class to test the exception with a controller exception ?

This brings be to think about the whole backtrace cleaning strategy, is it really useful ? Can't we just, in the HTML e-mail, differenciate app and gem backtrace lines ?

Thanks for your feedback !

@vala
Copy link
Author

vala commented Feb 26, 2016

Before fixing the failing test for https://travis-ci.org/smartinez87/exception_notification/builds/112021045, I'd like that we address the questions in my PR ! Thanks !

@smartinez87
Copy link
Owner

hi @vala, thanks for this.

Maybe we can go with writing a separate test class to test the exception with a controller exception for now, to have the feature available.

@smartinez87
Copy link
Owner

Can you also please add an entry to the changelog with this, mentioning you and @chancancode as the authors?

smartinez87 and others added 9 commits March 10, 2016 11:51
This, I think, is the most common use-case for custom data, and
I think many people will benefit from a simple example.

[ci skip]
…ustom_data

A simpler example of custom data
…eption_notification into optional_backtrace_cleaning
@monfresh
Copy link

monfresh commented Nov 2, 2017

Hello. This has been open for over a year. Anything I can do to help get it merged? cc @vala @smartinez87

@vala
Copy link
Author

vala commented Nov 3, 2017

If you want to take the PR over don't hesitate, I'm unsure I'll be able to work on that soon. Sorry for the lack of follow-up on this

@FLarra
Copy link
Collaborator

FLarra commented Jan 30, 2019

@monfresh Would you mind taking care of this PR if it's still a desired feature for you. Otherwise, I'll try to move this forward.

Thank you

@FLarra FLarra added the WIP Work in progress label Jan 30, 2019
@TylerRick
Copy link

I discovered this PR while looking for any existing issue or PR for adding backtrace cleaning for background_exception_notification. Currently backtrace cleaning is only done in the exception_notification that is called as part of a web request, but I couldn't understand why anyone wouldn't also want clean backtraces for background exception notification as well to be consistent.

So I am glad to find this PR makes both notifications clean backtraces and hope it gets merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants