Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update scopes documentation for Java and Android #4747
Update scopes documentation for Java and Android #4747
Changes from 10 commits
1bb105b
2621110
512c9e3
4cd11c7
85df56d
52945f7
1d1fbe0
014eaa2
70231d3
59072db
fb07118
74f8aa8
ba85cae
e146473
fb551f7
a1d24cf
e2de2e3
10b558e
bc7165a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to keep android as non supported due to https://github.com/getsentry/sentry-docs/pull/4747/files#r841714836
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! with this did you mean that you wanted to remove the mention of Kotlin as well as the local scopes stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for Android, since there's a global hub/scope, it does not make sense to use that integration, so we can remove from the android page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, that makes sense. As a counter point though, I was confused that it was mentioned for Java and not Android, which is why I added it to begin with. What do you think about mentioning somewhere in the Android setup docs that the Kotlin extensions aren't needed for Android?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, the content is not for Android at all, it even says:
Instead of Sentry's SDK for Android, which is the case for all the other Android integrations, I guess explaining something from another platform would also make confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marandaneto done in 10b558e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To go back to this thread though, I was confused and thought that the Kotlin extensions lib was needed for two reasons:
lib-ktx
library with extension functions focused on making the library more kotlin-like in the Kotlin world.Thus, it's really easy for someone who's not familiar with the platform but using Kotlin on Android to poke around the docs and accidentally think that they need the Kotlin extensions library, when they do not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, The goal is that
sentry-kotlin-extensions
get expanded and add extension functions to be "Kotlin friendly", but right now it only has theSentryContext
and this only makes sense for non-Android.E.g. https://github.com/getsentry/sentry-java/pull/1886/files
This would make sense for Android too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave this specific part for another PR, this will get addressed automatically when we land the 1st Kotlin function in the
sentry-kotlin-extensions
library for Android, which is gonna be very soon :)Thanks for the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, that makes sense.
Oooh, that would be handy.
Awesome! That sounds good to me!