Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set up more strict code styles and fix existing issues #4337

Merged
merged 1 commit into from Feb 11, 2020
Merged

Set up more strict code styles and fix existing issues #4337

merged 1 commit into from Feb 11, 2020

Conversation

kezhenxu94
Copy link
Member

Motivation:

Review code styles with some bots automatically.

Modifications:

  • Set up ReviewDog in GitHub Action to review code style.

  • Add more check rules to checkstyle plugin.

Result:

  • Obvious code styles can be reviewed and commented automatically.

@kezhenxu94 kezhenxu94 added the chore Chores about the project, like code cleaning up, typos, upgrading dependencies, etc. label Feb 9, 2020
@kezhenxu94 kezhenxu94 added this to the 7.0.0 milestone Feb 9, 2020
@kezhenxu94 kezhenxu94 force-pushed the chore/code-style branch 2 times, most recently from 135d485 to 136b2e3 Compare February 9, 2020 13:22
wu-sheng
wu-sheng reviewed Feb 9, 2020
View reviewed changes
.github/workflows/ci-it.yaml Outdated Show resolved Hide resolved
.github/workflows/ci-it.yaml Outdated Show resolved Hide resolved
.github/workflows/ci-it.yaml Outdated Show resolved Hide resolved
@kezhenxu94
Copy link
Member Author

Tests result look good

@kezhenxu94
Copy link
Member Author

kezhenxu94 commented Feb 9, 2020 •
edited

I've just added checks for @author tag and empty JavaDoc tags (@param, @return, etc.), which are the most obvious problems that we're facing from the current contributors, if I miss some, please let me know

@wu-sheng
Copy link
Member

wu-sheng commented Feb 9, 2020

I've just added checks for @author tag and empty JavaDoc tags (@param, @return, etc.), which are the most obvious problems that we're facing from the current contributors, if I miss some, please let me know

I think you should set them as an error, as removing the illegal ones once for all.

@wu-sheng
Copy link
Member

wu-sheng commented Feb 9, 2020

Does this review only cover the changes? I remember there are plenty of author, it didn't indicate them.

@codecov-io
Copy link

codecov-io commented Feb 9, 2020 •
edited

Codecov Report

Merging #4337 into master will decrease coverage by 0.65%.
The diff coverage is 33.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4337      +/-   ##
==========================================
- Coverage   26.93%   26.27%   -0.66%     
==========================================
  Files        1177     1177              
  Lines       25836    26698     +862     
  Branches     3688     3680       -8     
==========================================
+ Hits         6958     7014      +56     
- Misses      18258    19067     +809     
+ Partials      620      617       -3
Impacted Files Coverage Δ
...he/skywalking/apm/agent/core/os/ProcessorUtil.java 100% <ø> (ø) ⬆️
...m/toolkit/log/log4j/v1/x/TraceIdPatternLayout.java 0% <ø> (ø) ⬆️
...ywalking/apm/agent/core/plugin/EnhanceContext.java 0% <ø> (ø) ⬆️
...he/skywalking/apm/agent/core/plugin/PluginCfg.java 0% <ø> (ø) ⬆️
...ing/apm/agent/core/logging/core/WriterFactory.java 69.23% <ø> (ø) ⬆️
.../core/logging/core/coverts/AgentNameConverter.java 100% <ø> (ø) ⬆️
...ing/apm/agent/core/plugin/DynamicPluginLoader.java 0% <ø> (ø) ⬆️
...e/skywalking/apm/agent/core/jvm/gc/G1GCModule.java 0% <ø> (ø) ⬆️
...agent/core/plugin/match/MethodAnnotationMatch.java 0% <ø> (ø) ⬆️
...apm/agent/core/plugin/PluginResourcesResolver.java 0% <ø> (ø) ⬆️
... and 1121 more

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 fa526e5...5ddc90b. Read the comment docs.

@kezhenxu94 kezhenxu94 changed the title Review code styles automatically Set up more strict code styles and fix existing issues Feb 10, 2020
@wu-sheng
Copy link
Member

Related to #4272

@wu-sheng
Copy link
Member

Please fix the conflicts. Besides the unstable profile e2e(will be fixed soon), there are other tests failing, please recheck.

@kezhenxu94 kezhenxu94 force-pushed the chore/code-style branch 4 times, most recently from 8a69b86 to 002a735 Compare February 10, 2020 12:33
@wu-sheng
Copy link
Member

Seems still failing. :P

@kezhenxu94 kezhenxu94 force-pushed the chore/code-style branch 2 times, most recently from 7d351b3 to bd67f14 Compare February 10, 2020 14:45
@JaredTan95 JaredTan95 mentioned this pull request Feb 11, 2020
Merged
3 tasks
@wu-sheng
Copy link
Member

2790 files changed. :P How this could be review. Haha.

@kezhenxu94
Copy link
Member Author

kezhenxu94 commented Feb 11, 2020 •
edited

2790 files changed. :P How this could be review. Haha.

I can't even review myself 😢 , that's why I configure an incremental check before #4337 (comment)

FYI, I just configure the rules and hit Command+Option+L, use regular expression to remove @authors, they effect mostly in just JavaDoc and imports, so as long as the tests passed, it should be generally OK then

I'm Checking the failure of E2E

@wu-sheng
Copy link
Member

Don't worry about profile e2e, it has a code issue which has been fixed in #4335. Waiting CI confirmation to merge it.

@wu-sheng
Copy link
Member

Merged, and conflicts showing up. Please fix, let's make this PR merged as next to avoid endless conflicts.

wu-sheng
wu-sheng previously approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirm by auto tests only.

@kezhenxu94 kezhenxu94 force-pushed the chore/code-style branch 3 times, most recently from 5ddc90b to 60c597e Compare February 11, 2020 05:50
@kezhenxu94
Set up more strict code styles and fix existing issues
8c31743 
Motivation:

Review code styles with some bots automatically.

Modifications:

Set up ReviewDog in GitHub Action to review code style.

Add more check rules to checkstyle plugin.

Result:

Obvious code styles can be reviewed and commented automatically.
@wu-sheng
Copy link
Member

Fail in spring case every time, strange though. :P

@wu-sheng
Copy link
Member

Is the agent compiling test failure from one to another because of network issue? The CI passed, so I assume the compiling should be fine.

@wu-sheng
Copy link
Member

[ERROR] Failed to execute goal on project feign-scenario: Could not resolve dependencies for project org.apache.skywalking.apm.testcase:feign-scenario:jar:1.0.0: Could not transfer artifact io.github.openfeign:feign-core:jar:9.2.0 from/to central (https://repo.maven.apache.org/maven2): Failed to transfer file https://repo.maven.apache.org/maven2/io/github/openfeign/feign-core/9.2.0/feign-core-9.2.0.jar with status code 500 -> [Help 1]

Seems like the Maven central is not stable.

@wu-sheng
Copy link
Member

Rechecked from locally, https://repo.maven.apache.org/maven2/io/github/openfeign/feign-core/9.2.0/feign-core-9.2.0.jar download is good.

@wu-sheng
Copy link
Member

@kezhenxu94 Do you agree we should merge this than waiting the CI fails again and again because of network issue?

@wu-sheng wu-sheng merged commit 5b255ba into master Feb 11, 2020
@wu-sheng wu-sheng deleted the chore/code-style branch February 11, 2020 09:11
@wu-sheng wu-sheng mentioned this pull request Feb 11, 2020
Closed
8 tasks
@kezhenxu94
Copy link
Member Author

@apache/skywalking-committers please update the codes locally and reimport the codeStyle.xml into your IDEA, thanks :)

@wu-sheng
Copy link
Member

@kezhenxu94 Send a notification mail to dev ml please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@wu-sheng wu-sheng wu-sheng approved these changes

Assignees
No one assigned
Labels
chore Chores about the project, like code cleaning up, typos, upgrading dependencies, etc.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kezhenxu94 @wu-sheng @codecov-io