-
Notifications
You must be signed in to change notification settings - Fork 642
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 #3515] Do some code optimization[AuthTokenUtils] #3644
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.
LGTM
@kyooosukedn Import the checkstyle of the local project and check the box. |
@@ -139,7 +139,7 @@ void list(HttpExchange httpExchange) throws IOException { | |||
}); | |||
|
|||
String result = JsonUtils.toJSONString(getClientResponseList); | |||
httpExchange.sendResponseHeaders(200, result.getBytes().length); | |||
httpExchange.sendResponseHeaders(200, result.getBytes(Constants.DEFAULT_CHARSET).length); |
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.
Here Constants.DEFAULT_CHARSET
needs to be imported:
org.apache.eventmesh.common.Constants;
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.
Thanks :)
Hi @Alonexc Is there something else that needs to be done? |
Please resolve conflicts. |
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.
I didn't see these changes required in #3515:
b. Remove redundant initializers.
c. Use 'Objects.requireNonNull()' to avoid an NPE.
d. Replace this if-then-else statement by a single return statement
e. Remove the extra "{}".
Please supplement.😊
Besides, please fix checkstyle warnings: https://github.com/apache/eventmesh/actions/runs/7481706701/job/20372145878
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3644 +/- ##
============================================
+ Coverage 17.55% 17.59% +0.04%
+ Complexity 1778 1774 -4
============================================
Files 797 797
Lines 29871 29786 -85
Branches 2581 2573 -8
============================================
- Hits 5243 5242 -1
+ Misses 24145 24063 -82
+ Partials 483 481 -2 ☔ View full report in Codecov by Sentry. |
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
Hi @Pil0tXia, I have corrected as you suggested. Thank u for the review :) |
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
...urity-auth-token/src/main/java/org/apache/eventmesh/auth/token/impl/auth/AuthTokenUtils.java
Outdated
Show resolved
Hide resolved
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.
LGTM
cc @Alonexc |
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3644 +/- ##
============================================
- Coverage 16.20% 0 -16.21%
============================================
Files 857 0 -857
Lines 30883 0 -30883
Branches 2685 0 -2685
============================================
- Hits 5005 0 -5005
+ Misses 25410 0 -25410
+ Partials 468 0 -468 ☔ View full report in Codecov by Sentry. |
@kyooosukedn Sorry for the delay. May you please resolve conflicts? 😊 @Alonexc May you please help merge this PR? |
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.
Re-approval. cc @Alonexc
Closing to re-trigger CI. |
Fixes #3515 .
Motivation
Code Optimization on AuthTokenUtils.java
Remove redundancy
Modifications
Remove duplicate code and redundancies
Documentation