-
Notifications
You must be signed in to change notification settings - Fork 624
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 #4088] Anonymous new can be replaced with lambda.[BroadCastSubClient] #4617
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.
Welcome to the Apache EventMesh community!!
This is your first PR in our project. We're very excited to have you onboard contributing. Your contributions are greatly appreciated!
Please make sure that the changes are covered by tests.
We will be here shortly.
Let us know if you need any help!
Want to get closer to the community?
WeChat Assistant | WeChat Public Account | Slack |
---|---|---|
![]() |
![]() |
Join Slack Chat |
Mailing Lists:
Name | Description | Subscribe | Unsubscribe | Archive |
---|---|---|---|---|
Users | User support and questions mailing list | Subscribe | Unsubscribe | Mail Archives |
Development | Development related discussions | Subscribe | Unsubscribe | Mail Archives |
Commits | All commits to repositories | Subscribe | Unsubscribe | Mail Archives |
Issues | Issues or PRs comments and reviews | Subscribe | Unsubscribe | Mail Archives |
if (msg.getHeader().getCommand() == Command.BROADCAST_MESSAGE_TO_CLIENT) { | ||
if (msg.getBody() instanceof EventMeshMessage) { | ||
String body = ((EventMeshMessage) msg.getBody()).getBody(); | ||
LogUtils.info(log, "receive message -------------------------------{}", body); |
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.
Please remove some unused imports, such as ReceiveMsgHook
. Additionally, my suggestion is to refer to the description in the issue: change " -------------------------------" to ": ".
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.
Modified the suggested changes. Do I have to create another PR?
Modified changes as suggested
if (log.isInfoEnabled()) { | ||
log.info("receive message : {}", body); | ||
} |
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.
By changing " -------------------------------" to ": ", there should not be any additional space between the word "message" and the colon ":".
Besides, why LogUtils
was substituted with logger?
} | ||
} | ||
} |
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.
Please mind Checkstyle.
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.
Done!
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.
Fixed codestyle. Sorry, I'm quite new to this, making a lot of mistakes at every step. Thanks for guiding
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.
@AXE-00 Please fix codestyle, remove useless import
fixed codestyle
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4617 +/- ##
============================================
- Coverage 16.93% 16.93% -0.01%
+ Complexity 1693 1692 -1
============================================
Files 788 788
Lines 29420 29420
Branches 2539 2539
============================================
- Hits 4983 4981 -2
- Misses 23972 23973 +1
- Partials 465 466 +1 ☔ View full report in Codecov by Sentry. |
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
Issue Fix: #4088
Replaced the anonymous inner class with a lambda expression for the handle method