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

Fix: Make lifecycle breadcrumbs logged in fragments customizable (#1734) #2299

Merged
merged 8 commits into from Oct 31, 2022

Conversation

ILikeYourHat
Copy link
Contributor

@ILikeYourHat ILikeYourHat commented Oct 14, 2022

📜 Description

Added a way for logging only a subset of a fragment lifecycle events through breadcrumbs.

💡 Motivation and Context

Issue: #1734

This change also introduces an API incompatibility for SentryFragmentLifecycleCallbacks. This is intentional: in my opinion, this class should have never been a part of public API. It is used only internally in FragmentLifecycleIntegration, and I can't think of a reason to use it in a different way. Feel free to discuss if you disagree, I am open to reverting this and providing full API compatibility if it's really needed.

💚 How did you test it?

Changed tests to include this new behavior.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, besides some suggestions. Thank you a lot for doing that 👍

Also, please add a changelog entry, so the CI passes, you can look up at other PRs on how we do that

@romtsn
Copy link
Member

romtsn commented Oct 18, 2022

We'd also have to update the documentation for the integration https://docs.sentry.io/platforms/android/configuration/integrations/fragment/, but we can take care of that if you don't have time.

@ILikeYourHat
Copy link
Contributor Author

We'd also have to update the documentation for the integration https://docs.sentry.io/platforms/android/configuration/integrations/fragment/, but we can take care of that if you don't have time.

No problem, I missed it previously

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2022

Codecov Report

Base: 80.21% // Head: 80.21% // No change to project coverage 👍

Coverage data is based on head (b763f28) compared to base (b516cf3).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2299   +/-   ##
=========================================
  Coverage     80.21%   80.21%           
  Complexity     3475     3475           
=========================================
  Files           247      247           
  Lines         12906    12906           
  Branches       1735     1735           
=========================================
  Hits          10352    10352           
  Misses         1894     1894           
  Partials        660      660           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@romtsn romtsn added the Hacktoberfest-accepted Accept for Hacktoberfest - will merge later label Oct 24, 2022
@ILikeYourHat
Copy link
Contributor Author

Do you need anything else from me about this PR? :)

@romtsn
Copy link
Member

romtsn commented Oct 27, 2022

Do you need anything else from me about this PR? :)

Nope, all good, just haven't had time to merge it. Will do so today. Thanks a lot for your contribution, appreciated 👍

CHANGELOG.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest-accepted Accept for Hacktoberfest - will merge later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants