Skip to content

Report exceptions occurring in Rake tasks and runner commands#536

Merged
smartinez87 merged 1 commit into
smartinez87:masterfrom
TylerRick:notify_from_rake_and_runner
Jun 26, 2023
Merged

Report exceptions occurring in Rake tasks and runner commands#536
smartinez87 merged 1 commit into
smartinez87:masterfrom
TylerRick:notify_from_rake_and_runner

Conversation

@TylerRick

Copy link
Copy Markdown

Resolves #369

@TylerRick TylerRick force-pushed the notify_from_rake_and_runner branch from b61ec4e to c06c36a Compare June 22, 2023 16:30
@TylerRick TylerRick changed the title Draft: Report exceptions occurring in Rake tasks and runner commands Report exceptions occurring in Rake tasks and runner commands Jun 22, 2023
@TylerRick

TylerRick commented Jun 22, 2023

Copy link
Copy Markdown
Author

All right, I think this is ready for review and/or merge now.

I guess we should update the Readme, too, and mention (or change) the fact that you need to:

  • Add require 'exception_notification/rails' below Bundler.require in your config/application.rb if you want to enable these behaviors.
  • Requiring it from an initializer is too late, because the rake_tasks callbacks have already been fired by that point.
  • ('exception_notification/rails' doesn't appear to be auto-required. Should it be?)
  • You can also require require 'exception_notification/rake' only, if for whatever reason you only want to enable that portion.

I didn't add tests for the runner portion, because that would require having a Rails app in our test environment, and I see that test/dummy, which used to include such an app, has been removed. (But I did manually test it from my app.)

If that is desired, could you please advise how you would prefer a Rails app to be added in tests? Should we manually generate one and add it back as test/dummy? It seems like that would suffer from bit rot and would be one more thing we'd need to maintain. Or should we add another gem that does some of that for us? (Don't know of one off-hand.)

Or should we look into doing it how they do it in railties/test/application/runner_test.rb or railties/test/commands/runner_test.rb, where they have these handy helpers like build_app (to generate a fresh Rails app on the fly) and

output = rails("runner", "puts 'hello world", allow_failure: true)       

? Those appear to come from their test/isolation/abstract_unit.rb file, which unfortunately is not included with the railties gem itself, so it seems that we would have to copy and paste that into our gem if we wanted to use it. (And even then, it might require other test dependencies...)

@smartinez87

Copy link
Copy Markdown
Owner

Thank you @TylerRick for the PR, it looks great!

As you say, having test/dummy app was painful to maintain, so no need to go down that path again.
Can you please add the needed change in the Readme for this?

@TylerRick TylerRick force-pushed the notify_from_rake_and_runner branch from c06c36a to 584f25c Compare June 24, 2023 01:29
@TylerRick

Copy link
Copy Markdown
Author

Okay, I took a stab at updating the docs!

I also added a note to the generated initializer (for people who don't read docs).

@TylerRick TylerRick force-pushed the notify_from_rake_and_runner branch from 584f25c to 7827536 Compare June 24, 2023 02:39
@smartinez87 smartinez87 merged commit 2d87c69 into smartinez87:master Jun 26, 2023
@smartinez87

Copy link
Copy Markdown
Owner

thank you @TylerRick, amazing job!

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.

Merge exception_notification-rake gem

2 participants