[#658] Modify checkstyle configuration

[jamessspanggg]

Fixes #658

The checkstyle for Java files does not check for line-wrap indentation
length.

Let's add the configuration to set line-wrap indentations to be 8 
spaces.

[jamessspanggg]

@fzdy1914 @jylee-git @eugenepeh may I ask if there's any other modifications needed for the existing checkstyle?

[fzdy1914]

No other suggestion from my side.

[eugenepeh]

Good catch, didn't realized that line wrap haven't been included in checkstyle.
No comment on my end but can check the Addressbook's checkstyle in case we missed out anything.

[jamessspanggg]

No comment on my end but can check the Addressbook's checkstyle in case we missed out anything.

Went through with the OSS Java coding standard and every check seems to be there.

Comparing with the addressbook's checkstyle.xml, they set warning severity for some invalid coding standards where RepoSense did not set as warning severity.

Also, should I create an issue/PR in AddressBook as they are also missing the 8 space line wrap indentation check? [Update: done so]

[Tejas2805]

I am not sure about this rule for line-wrap, therefore just want to confirm. Shouldn't indentation and line wrap indentation be same?

[eugenepeh]

I am not sure about this rule for line-wrap, therefore just want to confirm. Shouldn't indentation and line wrap indentation be same?

Line wrap is supposed to use double the indentation to distinct itself as line wrap and not the subordinate of previous line

[eugenepeh]

Comparing with the addressbook's checkstyle.xml, they set warning severity for some invalid coding standards where RepoSense did not set as warning severity.

The rules on addressbook are meant to be more relax for flexible learning. Its not strictly enforce like RepoSense as we're doing actual production

[anubh-v]

What is our required indentation level when we have multiple consecutive line wraps?

For example,

private static final Path GROUPS_TEST_CONFIG_FILES = new File(RepoConfigurationTest.class.getClassLoader()
            .getResource("RepoConfigurationTest/repoconfig_groups_test").getFile())
            .toPath();

Should the line toPath(); be indented 8 spaces to the right relative to .getResource(... or should it be on the same level as .getResource(... ?

With the new setting, checkstyle recommends that toPath(); should be on the same level as .getResource(...
This seems ok to me.

[jamessspanggg]

Should the line toPath(); be indented 8 spaces to the right relative to .getResource(... or should it be on the same level as .getResource(... ?

It should be 8 spaces to the right relative to the first line, aka private static final...

[anubh-v]

Based on some testing I did, seems that the lineWrappingIndentation check will cause an error if we have fewer than 8 spaces' indentation. However, if we have more than 8 spaces' indentation, no error is thrown.

Is this intended?

[jamessspanggg]

Is this intended?

Good catch. Think we should set it to be exactly 8 spaces.

[jamessspanggg]

It seems that the lineWrappingIndentation will raise an error if we have fewer than 8 spaces' indentation. However, if we have more than 8 spaces' indentation, no error is thrown.

I've tried to set the exact 8 space indentation rule using forceStrictCondition = "true". However, with this rule, further line-wraps will still need to be exactly 8 spaces from the first line (examples shown below).

Success checkstyle

config.setIgnoreCommitList(
                Collections.singletonList(
                new CommitHash(FAKE_AUTHOR_BLAME_TEST_FILE_COMMIT_08022018_STRING.substring(0, 8))));

Failed checkstyle, the new should be at same indentation as Collections.single...

config.setIgnoreCommitList(
                Collections.singletonList(
                        new CommitHash(FAKE_AUTHOR_BLAME_TEST_FILE_COMMIT_08022018_STRING.substring(0, 8))));

This is a limitation with the Java checkstyle where it can't detect further line-wraps. I guess we can stick with the current "loose" rule for 8 space line wrap indentations to take into account these situations?

[anubh-v]

I guess we can stick with the current "loose" rule for 8 space line wrap indentations to take into account these situations?

I'm ok with this approach.