-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Refactor LicenseFragmentHelper. #8682
Refactor LicenseFragmentHelper. #8682
Conversation
dialog.dismiss() | ||
} | ||
alertDialog.setNeutralButton(R.string.open_website_license) { _, _ -> | ||
}.setNeutralButton(R.string.open_website_license) { _, _ -> |
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 would keep this on a new line, IMO.
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.
Also, I don't believe the default parameter commit is particularly useful.
It allows the method overload to be removed.
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'm aware, it's just that IMO it didn't really do anything useful here, considering that both showLicense
have different block arguments for the "backend" showLicense
's AlertDialog. I think the previous one is simpler to read, even if it makes the class have a few more lines. If it's merged as is, then the block's default parameter (with its arguments) is just overridden by the block arguments set in showLicense(Context, SoftwareComponent)
. I do understand the reasoning behind it, I just think that it makes it overall harder to interpret.
I can't explain very well right now, my brain is kinda fried.
@@ -1,3 +1,5 @@ | |||
@file:JvmName("LicenseFragmentHelper") |
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.
None of these are used in Java, so I don't believe we need this.
Also, I don't believe the default parameter commit is particularly useful. |
5e193c1
to
779f591
Compare
779f591
to
cfd8daa
Compare
5345652
to
231785b
Compare
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.
Tested, works, thanks. The build was failing because of ktlint, I fixed that.
231785b
to
4b7de86
Compare
Kudos, SonarCloud Quality Gate passed! |
What is it?
Description of the changes in your PR
LicenseFragmentHelper
.Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence