Skywalking: Codes clean up plan and set up more strict code style rules

Created on 19 Jan 2020  路  9Comments  路  Source: apache/skywalking

Since there are more and more contributors joining the project, we are facing the fact that different contributors have their own code style preferences, and some of the codes are changing frequently due to code styles (especially the imports, spaces, etc.), so I'm proposing to do some cleaning works and set up more strict code style rules.

PROS

Codes would be much more clean, and code styles are unified, more comfortable to maintain a such big codebase.

CONS

Many codes will be modified in the cleaning process.

TODO

  • [ ] Use Lombok as many as possible, reducing boilerplate codes, such as getters, setters, constructors, and many try-catches;
  • [ ] Set up more strict code style rules, including:

    • [x] avoid star import, such as import java.util.*;

    • [x] set up imports order, like java. comes first, org.apache. comes second, etc.

    • [x] require spaces after ,, before {, and between operators, etc.

    • [x] don't allow >= 2 consecutive blank lines, etc.

    • [ ] use lambda whenever possible

    • // ...... your suggestions here

  • [x] Set up the code style in the IntelliJ IDEA, and export it to a codeStyle.xml that can be imported by other contributors

I hope we could do these things in a single PR, so that:

  • the style changes only once, and the "real code changes" can be tracked without finding >1 unnecessary commit in the history;
  • it's easy to review if only code styles are changed

And I suggest the works could be done right before our next release, after all(or most) pull requests have been merged, and the codebase is frozen, to avoid conflicts;

Anyone who wants to undertake this work, or has different ideas, please leave your comment; if @apache/skywalking-committers have more suggestions, please supplementing the TODOs above by editing this directly, others can create new comment below, thanks;

good first issue

All 9 comments

For the rules you want to set, I am good with them.

And I suggest the works could be done right before our next release, after all(or most) pull requests have been merged, and the codebase is frozen, to avoid conflicts;

I think we may not need to wait for this. It is hard to tell the time, whenever this is ready, please submit the pull request. As we check all these in the CI process, other PRs will be required to change.

+1

Two suggestions

  1. Remove all authors, and don't allow to use it, due to the project belongs to the ASF.
  2. Make more strict rules for LOG, logger, LOGGER.

Rule, don't use Deprecated API. From https://github.com/apache/skywalking/pull/4318#issuecomment-581966104

If @JohnNiang or any other contributor is interested in this, feel free to leave a comment and open pull requests to fix, thanks :)

@kezhenxu94 Which things left here? As #4337 has been merged.

@kezhenxu94 Which things left here? As #4337 has been merged.

Have updated the TODOs in the first comment of this issue, those left unchecked are unsupported by Checkstyle plugin by default, may need some customization, low priority though

Agree, but is it reasonable to keep this issue open? As there is no tool could do this.

@wu-sheng I'm totally ok to close it for now

Was this page helpful?
0 / 5 - 0 ratings

Related issues

liyongxin picture liyongxin  路  5Comments

wu-sheng picture wu-sheng  路  5Comments

alishangtian picture alishangtian  路  3Comments

dingsongjie picture dingsongjie  路  4Comments

GreyZeng picture GreyZeng  路  5Comments