Skip to content

remove lateinit#396

Merged
tobiasKaminsky merged 2 commits into
masterfrom
lateinit
Feb 10, 2020
Merged

remove lateinit#396
tobiasKaminsky merged 2 commits into
masterfrom
lateinit

Conversation

@tobiasKaminsky

@tobiasKaminsky tobiasKaminsky commented Feb 4, 2020

Copy link
Copy Markdown
Member

Fix #395

Based on #394, so let us first merge #394, then I'll rebase this to master, for proper check, etc.
(I just did it this way to have it clean and review can start right away)

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

Comment thread src/androidTest/java/com/owncloud/android/AbstractIT.java

account.loadCredentials(context);
client.credentials = account.getCredentials().toOkHttpCredentials();
val credentials = account.getCredentials().toOkHttpCredentials();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine adding val via lombok in java code but we should then decide to do so. What do you think @tobiasKaminsky @ezaquari? Just asking since this adds a kotlin language feature to java code. Not sure if we want to mix language concepts. I am not having a strong opinion on this. We should then just document such a rule.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this was just a mistake, as I thought I am writing Kotlin code.
I will change this.

@ezaquarii

ezaquarii commented Feb 4, 2020 via email

Copy link
Copy Markdown
Contributor

@AndyScherzinger

Copy link
Copy Markdown
Member

@ezaquarii see https://projectlombok.org/features/val - and I don't know why they introduced this feature

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

You can discuss this, if you want ;-)
But I already removed this lombok val and we really should not mix this up…

@AndyScherzinger

Copy link
Copy Markdown
Member

It seems to me we are all on the same page, so all good 👍

@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 31a41df to 78ec45a Compare February 4, 2020 23:11
val client: OkHttpClient
class NextcloudClient(var baseUri: Uri,
var userId: String,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that by default, Kotlin members are public, contrary to Java, where default scope is package private.

We shall close the access as much as possible. If we need to keep them accessible insidde tests, internal scope can be used to limit access to library only.

@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch from 78ec45a to eb51e04 Compare February 5, 2020 22:14
AndyScherzinger
AndyScherzinger previously approved these changes Feb 6, 2020
@ezaquarii
ezaquarii force-pushed the ezaquarii/fix-nextcloud-client-exception-handling branch 2 times, most recently from 220afbb to 803df96 Compare February 6, 2020 16:18
@tobiasKaminsky
tobiasKaminsky dismissed AndyScherzinger’s stale review February 7, 2020 08:14

The base branch was changed.

@tobiasKaminsky
tobiasKaminsky changed the base branch from ezaquarii/fix-nextcloud-client-exception-handling to master February 7, 2020 08:14
do not use Lombok's val
limit scope

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Lint

TypemasterPR
Warnings00
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings14
Correctness Warnings39
Internationalization Warnings6
Malicious code vulnerability Warnings7
Multithreaded correctness Warnings3
Performance Warnings17
Security Warnings1
Dodgy code Warnings58
Total145

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings14
Correctness Warnings39
Internationalization Warnings6
Malicious code vulnerability Warnings7
Multithreaded correctness Warnings3
Performance Warnings17
Security Warnings1
Dodgy code Warnings58
Total145

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Lint

TypemasterPR
Warnings00
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings14
Correctness Warnings39
Internationalization Warnings6
Malicious code vulnerability Warnings7
Multithreaded correctness Warnings3
Performance Warnings17
Security Warnings1
Dodgy code Warnings58
Total145

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings14
Correctness Warnings39
Internationalization Warnings6
Malicious code vulnerability Warnings7
Multithreaded correctness Warnings3
Performance Warnings17
Security Warnings1
Dodgy code Warnings58
Total145

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

Test was successful, but codecov timed out, thus merging.

@tobiasKaminsky
tobiasKaminsky merged commit 5eae231 into master Feb 10, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the lateinit branch February 10, 2020 10:54
@AndyScherzinger AndyScherzinger added this to the NC Android lib 2.1.0 milestone Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NextcloudClient misuses lateinit to allow uninitialized variables

4 participants