Skip to content

Isolate ActiveJob error handling methods#1708

Closed
mbobin wants to merge 1 commit intogetsentry:masterfrom
mbobin:master
Closed

Isolate ActiveJob error handling methods#1708
mbobin wants to merge 1 commit intogetsentry:masterfrom
mbobin:master

Conversation

@mbobin
Copy link
Copy Markdown

@mbobin mbobin commented Feb 1, 2022

Thanks for your Pull Request 🎉

Please keep these instructions in mind so we can review it more efficiently:

  • Add the references of all the related issues/PRs in the description
  • Whether it's a new feature or a bug fix, make sure they're covered by new test cases
  • If this PR contains any refactoring work, please give it its own commit(s)
  • Finally, please add an entry to the corresponding changelog

Other Notes

  • We squash all commits before merging
  • We generally review new PRs within a week
  • If you have any question, you can ask for feedback in our discord community first

Description

Describe your changes:

This pull request refactors the ActiveJobExtensions module to prevent its methods from being leaked into the ActiveJob object. The reason behind this change is that I'm trying to run sentry-raven and sentry-rails side by side while doing the upgrade and both gems define these methods.

@mbobin
Copy link
Copy Markdown
Author

mbobin commented Feb 8, 2022

@st0012 could you have a look at this PR, please? 🙏

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 8, 2022

@mbobin I'm neutral for this type of change but I don't think this one is justified, because compatibility with sentry-raven isn't something we support.

If it's a temporary need, I'd recommend you to have a fork of sentry-raven with the similar changes. That way, once you finished testing you can just drop it.

@mbobin
Copy link
Copy Markdown
Author

mbobin commented Feb 8, 2022

@st0012 I thought this benefits this gem more because it stops polluting ActiveJob's descendants with Sentry specific methods. Let's take for instance the changes from v4.8.2, this is where capture_and_reraise_with_sentry's signature was changed from 3 to 2 arguments and this can be seen as a breaking change because they are exposed as public methods in the job classes and people might use them.

@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 12, 2022

@mbobin ok I get the point. I'll make a PR for it 👍

@st0012 st0012 mentioned this pull request Feb 12, 2022
Merged
@st0012
Copy link
Copy Markdown
Contributor

st0012 commented Feb 12, 2022

@mbobin I've opened #1725 for the same purpose. So I'm going to close this now. Thanks for proposing this change.

@st0012 st0012 closed this Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mbobin @st0012