-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Deprecate abstract coroutine context keys #4522
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
Open
dkhalanskyjb
wants to merge
1
commit into
develop
Choose a base branch
from
dkhalanskyjb/deprecate-polymorphic-keys
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
That's a bit of a nuisance.
I would consider coupling this change with introduction of
.dispatcherand.dispatcherOrNullextensions onCoroutineContext(not sure about theCoroutineScope).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 see what you mean, but this pattern is very niche anyway: https://grep.app/search?f.lang=Kotlin®exp=true&q=%5B%5E+%5D%5C%5BCoroutineDispatcher%5C%5D
In addition, in most cases, using
ContinuationInterceptorinstead ofCoroutineDispatcherdoesn't make any functional difference (or even makes the code a bit more flexible), which means directing the few existing cases to.dispatcheris not ideal.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've figured, but still see it as a good idea:
capitalize) for users who do use thatThere 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.
My 2 cents:
The fact that something is niche shouldn't be the reason to dismiss/remove it.
It should only be a secondary factor, if a main reason justifies the removal/dismissal of something, IMO.
Also, beware that
grep.appisn't finding all usages that GitHub search would.For this case, your link leads to 66 usages, while I got 338 with public-only GitHub search: https://github.com/search?q=%22t%5BCoroutineDispatcher%5D%22+language%3AKotlin&type=code
If the measuring tool you use is minimizing usage of stuff, you'll end believing things are much more niche than they actually are.
That would risk churning more Kotlin devs than you would think if actions are made based on flawed data.
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.
Hi Louis,
Since you're raising the topic of grep.app's reliability for the second time, I assume you'd appreciate an extended description of the methodology.
Yes, you are right, if something is barely used, it's not a reason to remove it or make it intentionally inconvenient. The criterion for inclusion of an API is that it should either solve a common need or not be easily expressible, but for removal, the harm of the API must outweigh its popularity. A terribly harmful API may get modified or removed even at the cost of far-reaching changes, whereas to remove an API that's just non-idiomatic and has a better alternative, it's enough for it to be niche.
grep.app is indeed not finding all usages. Neither does github, as there is a lot of code that's not on github at all. This is not a problem for our analysis, as the goal is not to evaluate how many codebases will be affected, but what fraction of them correspond to which measure of popularity. https://grep.app/search?q=kotlinx.coroutines.Job or https://grep.app/search?q=kotlinx.coroutines.flow.combine are examples of widely used APIs. https://grep.app/search?q=kotlinx.coroutines.channels.toList is niche.
The distribution of hits on grep.app is a bit different from the one on GitHub, as grep.app filters out duplicate files. If a user copy-pastes a library into their project, it results in additional hits on GitHub but not on grep.app. This means that not only absolute numbers will be different between GitHub and grep.app, but also the relative ones. I link to grep.app, as I believe that copies of libraries are not important for our analysis, since users are unlikely to maintain them on their own, so grep.app's results are more representative.
Another reason to use grep.app is to understand the usage patterns. This is often much more significant than it is to just count usages, as that way, we can learn more about why people are using an API at all and whether we can provide them with a better alternative. Removal of duplicates on the grep.app's side helps us more conveniently sift through the files.
To sum it up, yes, we are aware of the tool's limitations, which is why we would never say "this change will affect 100 codebases." We have no way of knowing. What we can fairly confidently say is, "the usage is dominated by uncommon requirements of a few large foundational codebases that have the resources to fix this."
Please let me know if this answers your doubts.
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.
Hello Dmitry,
Thanks for the answer!
I appreciate it, and it makes sense to me.