-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add Marten integration #90
base: master
Are you sure you want to change the base?
Conversation
Hey @Sija! Any chance you could review this? 😊 |
Hi @ellmetha, thanks for this contribution! Since you did quite a job here, I'll need a bit of time in order to review it properly, and I have little time atm. As for what you did write:
Great to hear that, I'm happy it works for you :) ❤️
I'm not sure I follow, since most of the other integrations is following the middleware pattern.
There are no other integration specs (sadly), so you're blazing the trail here :) This one is bit tricky though, since you can't add all of the frameworks together as development dependencies, since they'll conflict with each other. I'm not sure how to solve it - that's why there was no integration specs ;) I'd prefer to have either a way to test all of the integrations (even if there's no specs written, yet) or to leave the specs out altogether, kinda all-or-nothing approach. If you'd have an idea how to approach it, considering what I've written I'm all ears :) |
Thanks for your reply @Sija! 🙏
What I meant is that the middleware mechanism that is provided by Marten does not leverage the regular
Maybe we could extract the specs related to integrations in specific "sub-project" folders that would each have a dedicated |
I see now, thanks for the explanation!
Sounds good, I'm thinking of creating a GitHub organization for raven, so these could live as a separate repos - being also a good demo of the integration usage. WDYT? |
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
That's a good idea. I think long term this would definitely be the simplest solution! 🙂 |
@ellmetha yup, I think so too 👍 I'll go ahead and create an org + prepare required refactors in order to accommodate the new approach. I'll ping you once I'm done. |
First of all, thanks for this great shard!
This Pull Request adds an integration for the Marten web framework.
I have been using raven.cr from quite some time for the Marten website, and I thought it would be nice to ensure that the middleware I created to have automatic error reporting is provided by this shard directly as an integration (like this is the case already for HTTP handlers and other frameworks). This will make it easier for people to use Sentry as part of Marten apps.
Unlike other integrations, Marten relies on a middleware mechanism to "hook" into the request/response lifecycle (and HTTP handlers are not exposed to developers using the framework). As such, this pull request does not reuse the existing logic provided by
Raven::HTTPHandler
.It should be noted that I also added specs for the integration, which led me to add
martenframework/marten
to the development dependencies. Let me know if this isn't the right approach for testing integrations in this shard!