-
Notifications
You must be signed in to change notification settings - Fork 625
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.
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
[ISSUE #3855] General code simplification #3856
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pandaapo IMO, If you overuse the static import feature, it makes the program unreadable and unmaintainable.
Thanks for your review. It is indeed controversial about In consideration of "use it when you require frequent access to static members from one or two classes" in Guidance, should I revoke all of them? Or revoke some except for the follows(>= 3 times access to static members of a class)? eventmesh/eventmesh-common/src/main/java/org/apache/eventmesh/common/protocol/http/HttpCommand.java Lines 22 to 24 in 21f0bb9
Lines 23 to 35 in 21f0bb9
Lines 38 to 40 in 21f0bb9
In addition, I think it will be better if community can specify when to use |
Codecov Report
@@ Coverage Diff @@
## master #3856 +/- ##
============================================
- Coverage 16.91% 13.92% -3.00%
+ Complexity 1410 1291 -119
============================================
Files 588 570 -18
Lines 25743 28806 +3063
Branches 2376 2806 +430
============================================
- Hits 4355 4011 -344
- Misses 20954 24422 +3468
+ Partials 434 373 -61
... and 231 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@pandaapo please fix the conflicts. |
IMO, revoke a portion except for accessing 3 or more static members of a type is better. @pandaapo |
It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback. If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3856 +/- ##
============================================
- Coverage 16.91% 13.92% -3.00%
+ Complexity 1410 1291 -119
============================================
Files 588 570 -18
Lines 25743 28806 +3063
Branches 2376 2806 +430
============================================
- Hits 4355 4011 -344
- Misses 20954 24422 +3468
+ Partials 434 373 -61 ☔ View full report in Codecov by Sentry. |
I agree with "use it when you require frequent access to static members from one or two classes". The class name of a static import member is helpful to indicate the use of the member. Please resolve conflicts~ |
It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback. If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh. |
Fixes #3855.
Motivation
see "Enhancement Request" in issue related.
Modifications
see "Describe the solution you'd like" in issue related.
Documentation