FindBugs and PMD appeased#330
Conversation
|
This is for PR #322 |
|
Hi @iskradelta thank you very much for this PR! 🎉 - @tobiasKaminsky can you have a look/test drive since I am currently away from my dev maschine for the next couple of days... cc @djubreel since you are also working on findbug/pmd/etc. corresponding issue: #322 |
|
Now PMD is appeased, squashed many warnings/errors, there was about 190-200ish at start, and about 20-30ish were suppressed by annotation, mostly duplicate string literals which would cause the code to be unreadable, and src/third_party added to exclude pattern. What else needs to be done? Do we have common code formatting checks in CI? |
|
Great work! I will have look tomorrow.
Currently not. We want to switch to the "default" code formatting found in AndroidStudio with line break at 120. (https://github.com/nextcloud/android/blob/master/CONTRIBUTING.md#android-studio-formatter-setup) |
| } | ||
|
|
||
| public static boolean getOnlyOnDevice(){ | ||
| public static boolean isnlyOnDevice(){ |
There was a problem hiding this comment.
typo --> isOnlyOnDevice
| if (inChannel != null) { | ||
| inChannel.close(); | ||
| } | ||
| outChannel.close(); |
There was a problem hiding this comment.
shouldn't the null check be kept for outChannel also?
|
@tobiasKaminsky please also have a look as like yo mentioned. I did and am fine with the changes, will have to run a test though. Other than that 👍 |
|
Looks good. |
|
Sounds good to me! 👍 |
|
The branch has conflicts Btw :( |
|
@iskradelta can you resolve the conflicts? (else it can't be merged) |
|
Sorry a bit late response. Uh, a little help here, Ive resolved the conflicts, rebased these commits ontop of master, and force pushed to iskradelta/android.git but it doesnt update this pull-request here? Or does it? |
|
it does - thanks! :) 👍 PRs are automatically changed since the originating branch got the changes 😃 |
|
@tobiasKaminsky did you test the PR? My test was negative (file listing always empty on any account on any server) on my phone but I will do another test after cleaning the installation (jumping back an d forth between DB versions, so so there are different aspects that might have an impact here). |
|
Strange, for me it is also not working on a fresh clean emulator :/ |
AndyScherzinger
force-pushed
the
master
branch
from
11c146c to
821dd1b
Compare
|
@tobiasKaminsky I found the empty list issue a |
|
@iskradelta @tobiasKaminsky I found another issue. Navigation via the drawer menu from "Uploads" to "Settings" crashes the app with a NPE. Edit: now fixed by b5f995b |
|
@tobiasKaminsky I'd say id is ready for beta (could you merge, or should I) but should survive there first before we merge it to master. |
|
@AndyScherzinger please do it. I am currently busy ;-) ;-) |
Some warnings have been supressed.
Some warnings have been supressed.
Some warnings have been supressed.
Some warnings have been supressed.
|
Imho ready to merge @tobiasKaminsky 👍 |
|
Hi @iskradelta, Thanks for yet another great PR pushing the code quality of the app! 🎉 |
4 participants
Starter issue. Working on satisfying PMD next.