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

[ISSUE #4763] Do some code optimization.[EventMeshCommon] #4764

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[ISSUE #4763] Do some code optimization.[EventMeshCommon] #4764

wants to merge 5 commits into from

Conversation

scwlkq
Copy link
Contributor

@scwlkq scwlkq commented Feb 1, 2024

Fixes #4763

Motivation

move EventMeshCommon class to Common module.

Modifications

Documentation

  • Does this pull request introduce a new feature? (no)

@codecov Codecov
Copy link

codecov bot commented Feb 2, 2024 •
edited

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 16.37%. Comparing base (648f3e9) to head (d751223).
Report is 6 commits behind head on master.

Files Patch % Lines
...ache/eventmesh/client/tcp/common/MessageUtils.java 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             master    #4764       +/-   ##
=============================================
+ Coverage          0   16.37%   +16.37%     
- Complexity        0     1734     +1734     
=============================================
  Files             0      856      +856     
  Lines             0    31266    +31266     
  Branches          0     2700     +2700     
=============================================
+ Hits              0     5121     +5121     
- Misses            0    25665    +25665     
- Partials          0      480      +480

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scwlkq scwlkq changed the title [#ISSUE 4763] Do some code optimization.[EventMeshCommon] [ISSUE #4763] Do some code optimization.[EventMeshCommon] Feb 3, 2024
Pil0tXia
Pil0tXia reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

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

Not all members in the EventMeshCommon class are shared across the entire project. Some members are used in the client.tcp package, while others are used in both the sdk module and the client package.

Therefore, it is not a good idea to directly move the EventMeshCommon class, which originally belongs to the client.tcp package, to the common module. It requires a more detailed and clear separation to unify all related usages.

@scwlkq
Merge branch 'fix_patch_4763' of github.com:scwlkq/eventmesh into fix…
49b04ae 
…_patch_4763

* 'fix_patch_4763' of github.com:scwlkq/eventmesh:
  move EventMeshCommon class to common module
@scwlkq
move some simple usage constants
a64c5d3
@scwlkq scwlkq closed this Feb 15, 2024
@scwlkq scwlkq deleted the fix_patch_4763 branch February 15, 2024 16:28
@Pil0tXia
Copy link
Member

Is there any obstacles in this PR?

@scwlkq scwlkq restored the fix_patch_4763 branch February 21, 2024 11:40
@scwlkq scwlkq reopened this Feb 21, 2024
@scwlkq
Copy link
Contributor Author

scwlkq commented Feb 21, 2024

Is there any obstacles in this PR?

Closed because I thought the issue was meaningless hahaha.

@scwlkq scwlkq requested a review from Pil0tXia March 25, 2024 14:01
Pil0tXia
Pil0tXia requested changes Apr 11, 2024
View reviewed changes
Copy link
Member

@Pil0tXia Pil0tXia left a comment

Choose a reason for hiding this comment

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

EventMeshCommon class is designed to store constants of EventMesh SDK. It should be kept in org.apache.eventmesh.client package although it is referenced by other protocols.

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

@Pil0tXia Pil0tXia Pil0tXia requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement] Do some code optimization.[EventMeshCommon]
2 participants
@scwlkq @Pil0tXia