Skip to content

Patch Rake::Task#execute instead of Rake::Application#display_error_message #27

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TylerRick
Copy link
Contributor

@TylerRick
Copy link
Contributor Author

TylerRick commented May 21, 2019

Someone should just fork Airbrake and replace Airbrake-specific things with ExceptionNotifier. That way we benefit from all the work they do upstream (Airbrake) — including all their test coverage!

TylerRick added a commit to TylerRick/exception_notification-rake_example_with_spring that referenced this pull request May 21, 2019
@TylerRick
Copy link
Contributor Author

Manually tested using https://github.com/TylerRick/exception_notification-rake_example_with_spring/tree/workaround_using_execute_patch

Would be nice to have integration tests like Airbrake has. I really wonder if forking Airbrake would be the most efficient route.

@TylerRick
Copy link
Contributor Author

TylerRick commented May 21, 2019

Another unexpected benefit of this PR is that it appears to use my local time zone now. (I'm not sure why this changed ... maybe ExceptionNotifier uses the timestamp key if it is passed in the data hash?)

Before:
A RuntimeError occurred in background at 2019-05-21 01:16:26 UTC :
Now:
A RuntimeError occurred in background at 2019-05-20 18:16:44 -0700 :

@nikhaldi
Copy link
Owner

Thanks @TylerRick - this is quite a fundamental change. Agreed that having integration tests would be really useful for this kind of thing. Do you have any ideas how to achieve that?

Can you also rebase this on top of master so we get a clearer diff (your other PRs are merged)?

@nikhaldi nikhaldi mentioned this pull request May 22, 2019
@TylerRick TylerRick force-pushed the patch_rake_task_instead_of_application branch from 1a4d16b to 5047c80 Compare May 22, 2019 04:07
@TylerRick
Copy link
Contributor Author

All right, it's rebased :)

It's basically just copying how they do it in https://github.com/airbrake/airbrake/blob/master/lib/airbrake/rake.rb, since they no longer do it the way they used to (the way that originally inspired this gem, I believe)...

@TylerRick TylerRick force-pushed the patch_rake_task_instead_of_application branch from 5047c80 to 1a72694 Compare June 20, 2023 20:01
@TylerRick
Copy link
Contributor Author

All right, it's rebased again!

@TylerRick
Copy link
Contributor Author

FYI: This PR (and this gem in general) may be redundant now that smartinez87/exception_notification#536 has been merged

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

Successfully merging this pull request may close these issues.

Doesn't work with spring
2 participants