search

se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 415 forks source link

Upgrade Checkstyle to 10.2 and update checks #133

Closed yhtMinceraft1010X closed 2 years ago

yhtMinceraft1010X commented 2 years ago

Fixes #124

There is one additional check that I wanted to implement: RequireEmptyLineBeforeBlockTagGroup. However, this particular line will fail the check: https://github.com/se-edu/addressbook-level3/blob/865de62540a50cf0690abc36a212eea1e0b4617a/src/main/java/seedu/address/commons/util/CollectionUtil.java#L15-L16

The problem here is that @see is treated like any other block tag despite this particular method having no Javadoc description of its own.

canihasreview[bot] commented 2 years ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

canihasreview[bot] commented 2 years ago

v1

@yhtMinceraft1010X submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/133/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 2 years ago

Thanks for this PR @yhtMinceraft1010X @se-edu/tech-team-level1 for your review please

yhtMinceraft1010X commented 2 years ago

Since this is updating the checkstyle file, it might be good to correct the broken link in line 8 as well. Current link: https://oss-generic.github.io/process/codingstandards/coding-standards-java.html Correct link: https://oss-generic.github.io/process/codingStandards/CodingStandard-Java.html

Is there any difference between https://oss-generic.github.io/process/codingStandards/CodingStandard-Java.html and the SE-EDU guide? I noticed the former link was last updated in 2018 and I think the SE-EDU version is more suitable.

damithc commented 2 years ago

Is there any difference between https://oss-generic.github.io/process/codingStandards/CodingStandard-Java.html and the SE-EDU guide? I noticed the former link was last updated in 2018 and I think the SE-EDU version is more suitable.

oss-generic is now defunct. Use se-education.org/guides instead.

codecov-commenter commented 2 years ago

Codecov Report

Merging #133 (f02972d) into master (0f842d8) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #133   +/-   ##
=========================================
  Coverage     72.15%   72.15%           
  Complexity      399      399           
=========================================
  Files            70       70           
  Lines          1232     1232           
  Branches        125      125           
=========================================
  Hits            889      889           
  Misses          311      311           
  Partials         32       32
Impacted Files Coverage Δ
src/main/java/seedu/address/ui/HelpWindow.java 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0f842d8...f02972d. Read the comment docs.

canihasreview[bot] commented 2 years ago

v2

@yhtMinceraft1010X submitted v2 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/133/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 2 years ago

v3

@yhtMinceraft1010X submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/133/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 2 years ago

Getting there 👍

Minor: the current commit message for v3 2/4:

There are some checks which enforce the Java basic and intermediate
coding standard as well as the Google Java Style Guide. As an example,
the MethodParamPad check ensures that a method or constructor name is
attached to its open parenthesis. The JavadocTagContinuationIndentation
check is for the Google style guide rule that continuation lines for
block tags are indented 4 or more whitespaces relative to the @.

The default throwsIndent value for the Indentation check is 4
whitespaces although the correct value should be 8.

Let's update the throwsIndent value, add the new checks and fix the
HelpWindow file for the JavadocTagContinuationIndentation check.

@yhtMinceraft1010X The justification of the rules can be more systematic. Perhaps revise the first paragraph to justify addition of each rule e.g., refer to the corresponding rule in the SE-EDU coding standard? Alternatively, list which were added on account of the basic rules, which for intermediate rules, which for Google style guide.

You can post the proposed commit message here for comments before updating the actual commit message.

yhtMinceraft1010X commented 2 years ago

You can post the proposed commit message here for comments before updating the actual commit message.

Here is my proposed commit message for v3 2/4:

There are some checks which enforce the Java basic and intermediate
coding standard as well as the Google Java Style Guide. 

The DefaultComesLast check ensures that default is the last statement 
for switch. The NoWhitespaceBeforeCaseDefaultColon check ensures 
that there is no whitespace between case and the colon or default and 
the colon. Both reference the example code for the basic rule on switch 
statement form.

The MethodParamPad check ensures that a method or constructor name 
is attached to its open parenthesis under the intermediate rule for line 
breaks.

The JavadocTagContinuationIndentation check is for the Google style 
guide rule 7.1.3: continuation lines for block tags are indented 4 or 
more whitespaces relative to the @.

All other newly added Javadoc checks are for the basic rule on the form 
of Javadoc comments.

The default throwsIndent value for the Indentation check is 4
whitespaces although the correct value should be 8.

Let's update the throwsIndent value, add the new checks and fix the
HelpWindow file for the JavadocTagContinuationIndentation check.
damithc commented 2 years ago

We can shorten like this (no need to use paragraph form if there is a better format):

Lets update checkstyle rules as follows, and update the code to match.

New rules:
* To match SE-EDU Java Coding Standard (basic):
  1. DefaultComesLast 
  2. NoWhitespaceBeforeCaseDefaultColon
  3. ...
* To match SE-EDU Java Coding Standard (intermediate):
  1. ...
* To match Google's Java Code Style:
  1. JavadocTagContinuationIndentation  

Updates to existing rules:
1. Update default throwsIndent value for the Indentation
   from 4 (incorrect) to 8 (correct).

Updates to code to match the above:
1. HelpWindow file: to match JavadocTagContinuationIndentation
damithc commented 2 years ago

To add, the explanation of each rule can be in the file itself, rather than the commit message.

canihasreview[bot] commented 2 years ago

v4

@yhtMinceraft1010X submitted v4 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/133/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 2 years ago

@se-edu/tech-team-level1 for your review please ...

Sorry @yhtMinceraft1010X , I missed this earlier.

damithc commented 2 years ago

Side note: As the four commits seem to be independent changes, I guess we can use 'rebase and merge' (i.e., no merge commit)?

vimuthm commented 2 years ago

Side note: As the four commits seem to be independent changes, I guess we can use 'rebase and merge' (i.e., no merge commit)?

Yes, a rebase makes sense