[Proposal] Remove CheckReturnValue on subscribe() overloads #6304
Conversation
|
Maybe you, but see the thread for plenty more cases where it's not useful :). Let's keep the discussion consolidated there |
|
This was discussed before. The no arg subscribe doesn't have it as being fire and forget, the rest do. |
|
That's not true actually
also requires it despite accepting an onSubscribe Consumer, and presumably that shouldn't require it by that logic either since a handling consumer is passed in. I understand it was discussed before, but I feel I've offered a good set of examples in the linked issue that highlight where the original reasoning has not accounted for. |
It is to be removed in 3.x: #5622 Also the issue here is a general conflict between one's individual coding style and the hard coded coding style of a library. So you either ignore warnings individually as Jake suggested or create a separate subscribe method/converter in your project where you can ignore it once. |
This seems like a really harsh stance to take :/. Any specific thoughts on the examples I raised? |
|
Talked with Zac offline and I understand his points now. We both have different valid ways of using RxJava and the annotation is a constraint that should not be enforced per se. I'd be willing to give up the annotation if we have proper open source toolings like Zac suggested that simulate the old behavior. |
|
And I'd be happy to contribute implementations (in existing or new projects) for error-prone and lint as a part of this :) |
|
This should have been part of some tooling all along, but then the lack of warnings out-of-box received by newcommers and beginners would have resulted in a lot of support requests. By the way, if tooling is created for this, it could have both the feature to add as well as remove via configuration for the entire project. |
|
my 2 cents: RxJava uses its own annotation for this, which means that it should be possible to disable it project (module) -wide without clashing with Thus, I'd rather keep it as is in RxJava (because there is value in it) and focus on tooling support for disabling such specific annotations. |
|
I think for Android Lint this might actually already be doable. For each issue there can be a regex defined. Might be enough to pass in the fully qualified name of the annotation. |
|
A custom annotation for this would work, but we'd probably have to deprecate the existing one vs. remove it since it's part of the public API right? @vanniktech have a link/sample? I didn't know that was an option! @akarnokd when you say
Could you elaborate a bit? I'm not sure I followed |
|
I meant like a config file: Some tooling would check this file and apply the annotation virtually to the given method signature or ignore it. |
|
Something like that might work (haven't tried it out myself): <?xml version="1.0" encoding="UTF-8"?>
<lint>
<issue id="CheckResult">
<ignore regexp="io.reactivex.annotations.CheckReturnValue"/>
</issue>
</lint> |
|
Let me see if ErrorProne's check allows for this. Unfortunately neither case covers the actual IDE tooling marking these (they don't read lint xml configurations and the intelliJ inspection doesn't support customization as far as I know) |
I'm sure this can be fixed. Just like custom lint checks got fixed with 3.3 |
|
Maybe. That was a bug and this would be a FR :). lint xml files are entirely a CLI implementation, and not part of the main lint API that the CLI and IDE plugins both implement |
|
Followup on error-prone - confirmed there's no way to configure this right now. I don't think focusing on disabling in other tooling is the right way to go about this, as it's much easier to enable custom tooling than it is to disable non-configurable existing tooling. |
|
One (me) could argue that changing a default behavior and requiring everyone who relies on it to reconfigure their tooling is also not great and hardly considerable as much easier :) |
|
That’s why I was proposing having new tooling available out the gate, so
anyone using lint or error-prone could just add these. If they’re already
using either of these, it should be a straightforward addition. If they
aren’t, then they likely weren’t using tooling for the old behavior anyway.
In either case - like I mentioned above, adding tooling is a matter of
configuration. Disabling tooling is nigh-impossible without forking
error-prone or lint to configure them in these cases. I’d rather err on the
side of flexibility than imposing something that basically can’t be
disabled.
…On Fri, Nov 16, 2018 at 1:43 AM Artem Zinnatullin :slowpoke: < ***@***.***> wrote:
One (me) could argue that changing a default behavior and requiring
everyone who relies on it to reconfigure their tooling is also not great
and hardly considerable as much easier :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6304 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABTEvrc9yjIqKbiN6QBtQFJXz4KpAbfnks5uvojFgaJpZM4YZBXR>
.
|
IntelliJ could read the lintConfig file from |
|
The problem is that's a significant feature request for functionality that doesn't exist today right? |
|
Maybe. Maybe not. It's worth asking for nonetheless ;) - https://issuetracker.google.com/issues/119660763 |
|
Even then, that still marries it to gradle android projects right? |
|
Probably yeah. Unless they find some universal way which would also work with other build systems. Feel free to suggest that in the issue since I'm only using Gradle. |
|
Yeah I think that's a good thing to file in general, but as a separate async thing. Mind if we continue with using a renamed annotation for rx? |
|
Yeah I'm in for having both (proper tooling support) and new tooling to flag this. You want to rename it so standard tools don't know the name and hence don't flag it, right? Custom checks like you proposed would read it then. |
|
Yeah rename, though we'd have to add a separate new one (and deprecate the existing one?) since it's part of the public API up to this point. Also - what should we call it? |
|
I guess yeah but then in the end tools still need a way to exclude the current annotation since we can't remove it, right? |
|
Oh no I meant you can't delete the class. You can still remove it from the methods. I was as a actually thinking it's fine in it's other uses, and maybe just make a custom 2nd one for like |
|
Sounds like a good tradeoff. |
ZacSweers
force-pushed
the
z/remoteCheckReturnValueAnnotations
branch
from
aa3f961 to
ced572d
Compare
Codecov Report
@@ Coverage Diff @@
## 2.x #6304 +/- ##
============================================
- Coverage 98.26% 98.22% -0.05%
+ Complexity 6284 6283 -1
============================================
Files 672 672
Lines 44992 44992
Branches 6223 6223
============================================
- Hits 44212 44193 -19
- Misses 244 255 +11
- Partials 536 544 +8
Continue to review full report at Codecov.
|
|
Rebased and switched over to a |
|
I'll create a project for that now 👍 |
|
You might have the possibility to reuse the existing Detector by creating your own and registering the new annotation and from there forward everything to the CheckResultDetector. That way you wouldn't need to duplicate all of the work. I've also filed https://issuetracker.google.com/issues/119763072 - dunno if that's something they are willing to do though. |
|
I've created a project with working checkers for both here: https://github.com/hzsweers/rxjava2-optionalcheckreturnvalue-checkers Going to try to get review from @vanniktech and a couple others separately, but they're mostly straightforward adaptation of existing checkers just for this new annotation |
|
Lint implementation looks good! |
|
Still opposed to this. Makes the common case worse for the benefit of the uncommon case. |
|
Counterexample: RxLifecycle is nigh-ubiquitous in Android apps and negatively affected by this (since lifecycle handling is handled via upstream takeUntil), would that still be considered an uncommon case? |
|
I think you vastly overestimate its use and the use of a view model (not
the library which has nothing to do with view models, of course, the actual
pattern) means you only have a single subscription inside a lifecycle
removing the need to use it anyway.
…On Fri, Nov 23, 2018, 4:52 PM Zac Sweers ***@***.*** wrote:
Counterexample: RxLifecycle is nigh-ubiquitous in Android apps, would that
still be considered an uncommon case?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6304 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEZr6kAGcNaPSHNc-aF2P7_rRrvHsks5uyG4KgaJpZM4YZBXR>
.
|
|
Maybe! Unfortunately neither of us can prove that one way or the other. What about any of the other cases I mentioned in #4878 (comment)? Especially considering RxJava is synchronous by default, it seems to me like there's quite a few "uncommon" cases. Maybe not enough to be "common", but it gets to a larger point of when static analysis tools should be imposed. To me, hard checks like this should only be imposed in black-and-white situations, like a factory or cases where it's otherwise pointless to call that method without capturing the result. This seems far from black-and-white. On top of that, it's a check that's not configurable in any built-in error-prone/lint tooling (it's even hardcoded into lint today) that would otherwise enforce it. I think I have an alternative idea, but I'd like to get thoughts on the above first |
Proposal PR for my followup comment in #4878 (comment)
Will close if the discussion raised there still settles on keeping them, just wanted to show a PoC of what the change scope would look like for reference.