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
[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All SDK Override Methods for Activities #18997
[Nullability Annotations to Java Classes] Add Missing Nullability Annotations to All SDK Override Methods for Activities #18997
Changes from all commits
346a8f8
be35881
1ebb789
064b8ee
91a632a
2d6457f
e31f056
c035d76
adb1e7f
56febf4
4737bfb
dfefa3b
7f4cbaa
4e9a2c1
a0da7cd
3e92a6d
8a38d8c
88d274f
2b54a8b
7a7fc66
3588f84
4152299
7753264
387099e
4db5b4e
cd60734
4b0d8a3
9a80cba
2f3028f
9f0e7fd
319c59a
0fde6d4
1389ddd
05fcb53
ac5ac3e
adf4744
d099dda
5262961
3f01766
8a4126e
710aed5
f40213e
54191a4
2a02c61
ec1c0b8
37b1368
dd8c1bb
0b7aded
8405c3b
5a7504a
922e9d9
0acc778
6068d3a
83b9261
975642e
15fefd7
a452dbf
00af6b7
4886871
e1dc640
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't feel strongly about this, but typically,
final
is used to prevent reassignment of the variable within the method. In this case, the method is rather simple, anditem
is only even used in the default call to the super method. Butfinal
parameters have no effect at the call site, so I'd generally be ok with keeping it - but in this case, since the method is very simple anyway, I'm also fine with removing it. 👍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.
The only reason I removed this
final
keyword is because I wanted every such method across the codebase to be consistent with each other.Thus, and because I found very few occurrences of such
final
usage on suchonOptionsItemSelected(...)
methods across the whole codebase, that it is better to remove it here as well, rather than adding it everywhere else. No matter, I think it is better for us to use a consistent signature for such methods. Wdyt? 🤔PS: Also, and since we don't have a documented way of using
final
, and since I rarely seen it being used, I think it is better to avoid adding it unnecessarily everywhere. Thus, I decided it might be better to remove it instead, whenever found on those@Override
methods.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 agree with this, but my point above is that
final
is not really part of the method signature. That's what I meant by "no effect at the call site".Since, in this instance, the usage is for such a simple method, I think it's ok to remove it. Generally speaking, though, the intention for using this is entirely compatible with keeping signatures consistent.
final
on a parameter only pertains to the usage of that reference within the method body, and is sometimes used to prevent reassignment of the variable. This can be a useful restriction in long / complex methods.On this same topic, there are many parameter renaming changes, in which the commit message mentions this was done for the sake of consistent method signatures, but parameter names are not part of a methods signature in Java. I didn't see any issue with the renamings that you did, but was curious to know your motivation. Does it relate somehow to Kotlin?
To play devils advocate, I could envision deviating from the overriden super class parameter names for clarity's sake - to I don't think we should have a rule that all parameter names must match that of the super class. Especially because some super methods have inappropriately terse parameter names.
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 agree with all your points @mkevins and apologies for the confusion here. What I meant with keeping signatures consistent wan't related to super vs implementing signature, but rather project wise, that is, so that when one is searching/copy-pasting such methods, that they are not seeing a mixture of both, some of them using a
final
, while others are not. Now, I agree that the usage offinal
is a case-by-case scenario, but on those specific cases I doubt that it was done like that (an assumption from my side). Instead, I think this was initial done purely because it is a best practice to usefinal
as much and as widely as possible, and then, that was blindly copy-pasted thereafter. However, since us usingfinal
vs not using it at all hasn't been discussed, or agreed upon, and since me seeing and experiencing us not usingfinal
during our day-to-day work with Java, that is, unless the compiler complains or warns us about it, I was thinking it is better to remove those usages as well. This way, IMHO, the next person to copy-paste such method would not unnecessarily copy-paste thefinal
as well, even if that means safer code... 🤷The other way around for me would have been to add the
final
to every such@Override
method. 🤔Having said all the above, it is easier for me to plain ignore such changes if this will make it easier for you to review. I am only doing those to increase the consistency and make sure that we are using such
@Override
method consistently across the codebase, just to have make us that bit more consistent. But, I can stop doing that if that makes you uncomfortable as removingfinal
is indeed less safe that having it there, or adding it everywhere missing, even if someone just copy-pasted that there from a previous such existing activity. 🤷