Apps-android-commons: Disabling LineLength Codacy check?

[domdomegg]

Personally, I prefer longer lines to weirdly broken up ones and don't see a point raising a load of LineLength errors with Codacy if it works, is understandable and Android Studio has horizontal scrolling and smart word wrap built in.

Thoughts?

[veyndan]

Usually long lines indicates that the line is too complex and should be splitted into more variables for example below the first one is over the 100 character limit whereas the second one isn't, but the second one is arguably more understandable.

((AppCompatActivity) getActivity()).setSupportActionBar((Toolbar) getActivity().findViewById(R.id.toolbar));

Toolbar toolbar = (Toolbar) getActivity().findViewById(R.id.toolbar);
((AppCompatActivity) getActivity()).setSupportActionBar(toolbar);

Also it depends on what screen size you're working with. When I'm working on my bigger monitors I don't mind long line lengths but when working on a 13" laptop where you have Android Studio taking up half the screen, I much prefer shorter lines as horizontal scrolling is always a pain.

[whym]

+1. I think we eventually want to reduce too long lines, but it doesn't feel like the top priority for us for now. In general, even if each of the checks makes sense, too many warnings in total could make people want to ignore them. (at least it does to me.) Commenting out and disabling less important checks can make others more visible.

[veyndan]

Another possibility is changing the severity of the other checks to be error so the build and Travis will fail if the rule is violated. For things such as incorrect indentation, incorrect spacing etc. I feel this could be very useful. It also means that reviewers don't need to look at the Checkstyle output so it can generally be ignored as all you have to see is if Travis failed or not which means that for people who want to see the check will still be able to see it and those who don't can just ignore the output.

Ideally overtime most things can be set to have a severity of error.

[whym]

I'd want separate signals for

A. Compilation errors
B. Coding style almost universally 'wrong'
C. Coding style not highly recommended but not too wrong

Travis serves as a signal of A, which is often a severer problem than B and C. Simply making Travis fail for B doesn't sound like a good idea, because the signal "Travis not happy" will become ambiguous. (It's true that you can visit the build log to see what the cause was, but requiring additional clicks kind of reduces the ease of use provided by Travis.)

Can Travis be customized to behave differently to different causes of failures?

[misaochan]

I would prefer that Travis be used for build/compilation errors, while keeping style checks for Codacy. IMO having the Travis build fail due to incorrect spacing/indentation would cause a lot of false alarm.

I'm ambivalent about the LineLength Codacy check - I agree that long lines are a pain to read on smaller screens, but I don't know how that should be weighed against having lines that are arbitrarily split up.

[misaochan]

Would like to bump this issue, as arbitrary splitting of lines is a huge headache IMO. This is especially due to the fact that oftentimes the lines themselves are not that long, but indentation (which is necessary for easy reading of code) makes them exceed the "ideal" length.

I strongly feel that in most cases, just having 1 line of code is much more readable than something like:

        switch (requestCode) {
            case REQUEST_PERM_ON_CREATE_STORAGE: {
                if (grantResults.length >= 1
                        && grantResults[0] == PackageManager.PERMISSION_GRANTED) {
                    backgroundImageView.setImageURI(mediaUri);

In this example, at a glance you would expect all of these lines to be nested statements, but the second last line is actually not.

[psh]

If you're using decent length method and variable names (for clarity) then the line can get pretty long too. I would favour either increasing the "ideal" length or dropping the check entirely but lean toward dropping the check.

[whym]

I feel like a higher threshold might be better than no warning at all. If we keep it high enough, we could pass nearly all intentional cases while catching some careless (and fixable) mistakes.

For example, "> 140 characters" would result in 92% reduction of detection (423 violations → 32). At 120, it would be 71% reduction. Here is a chart showing line length vs occurrences:

In UploadService.java, the violations of 140 character length are:

https://github.com/commons-app/apps-android-commons/blob/8dfe9d6bff952e2323025cfa5738c2a9dae9c5a3/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java#L83
https://github.com/commons-app/apps-android-commons/blob/8dfe9d6bff952e2323025cfa5738c2a9dae9c5a3/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java#L148
https://github.com/commons-app/apps-android-commons/blob/8dfe9d6bff952e2323025cfa5738c2a9dae9c5a3/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java#L246
https://github.com/commons-app/apps-android-commons/blob/8dfe9d6bff952e2323025cfa5738c2a9dae9c5a3/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java#L272

(I believe these will overflow on GitHub's web view for many screens/browsers.)

[psh]

@whym - you ignited my competitive nature - if we set the line length to 140, then 32 violations is totally doable to fix. Anyone else feel up for the challenge of shooting for zero? 😁

[misaochan]

Setting the line length to 140 sounds good to me! :)

[whym]

So it seems that we can close this issue with the 100 -> 140 change?

We might want to revisit this (and perhaps slightly decrease the threshold if we manage to achieve zero violations at 140) - I'd suggest a separate issue page for that.

[misaochan]

Sounds good. Thanks @whym ! :)