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

[refactor] refactoring methods replaceCryPlaceholder and replaceSmilingPlace #2832

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

[refactor] refactoring methods replaceCryPlaceholder and replaceSmilingPlace #2832

wants to merge 18 commits into from

Conversation

hasimmollah
Copy link

@hasimmollah hasimmollah commented Nov 23, 2024 •
edited
Loading

What's changed?

#2056

Issue 2056 was raised to refactor CollectUtil.java for more readability. Refactoring methods replaceCryPlaceholder and replaceSmilingPlace in CollectUtil.java for readability and to reuse common code extracted codes to replace special character. As this part is common for the scenario when json element is JsonObject or JsonArray in replaceCryPlaceholder and also for replaceSmilingPlace

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

@hasimmollah
Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmili…
97951c9 
…ngPlace in CollectUtil.java for readability and to reuse common code
@hasimmollah hasimmollah changed the title feature: Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmilingPlace in CollectUtil.java for readability and to reuse common code refactor: Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmilingPlace in CollectUtil.java for readability and to reuse common code Nov 23, 2024
@hasimmollah hasimmollah changed the title refactor: Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmilingPlace in CollectUtil.java for readability and to reuse common code [refactor] Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmilingPlace in CollectUtil.java for readability and to reuse common code Nov 23, 2024
@hasimmollah
Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmili…
390c66c 
…ngPlace in CollectUtil.java for readability and to reuse common code with comment
yuluo-yx
yuluo-yx previously approved these changes Nov 23, 2024
View reviewed changes
Copy link
Member

@yuluo-yx yuluo-yx left a comment

Choose a reason for hiding this comment

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

LGTM

@tomsun28
Copy link
Contributor

tomsun28 commented Nov 24, 2024 •
edited
Loading

hi, thanks for contribution!
Can you help to provide some unit test to ensure this work success? due this involves many times where it will be used.

@tomsun28 tomsun28 changed the title [refactor] Issue 2056 refactoring methods replaceCryPlaceholder and replaceSmilingPlace in CollectUtil.java for readability and to reuse common code [refactor] refactoring methods replaceCryPlaceholder and replaceSmilingPlace Nov 24, 2024
@tomsun28 tomsun28 added the good first pull request Good for newcomers label Nov 24, 2024
@hasimmollah
Copy link
Author

hasimmollah commented Nov 24, 2024 •
edited
Loading

Added few more test cases now to improve test coverage

@hasimmollah hasimmollah requested a review from yuluo-yx November 24, 2024 18:21
@tomsun28 tomsun28 added this to the 1.6.2 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuluo-yx yuluo-yx Awaiting requested review from yuluo-yx

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
good first pull request Good for newcomers
Projects
Apache HertzBeat (Incubating)
Status: To do
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.
6 participants
@hasimmollah @tomsun28 @yuluo-yx @zuobiao-zhou @Aias00 @Calvin979