Skip to content

866 - JacksonJsonBuffer - Overriding toString #1073

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
Merged
merged 2 commits into from
Sep 29, 2025
Merged

866 - JacksonJsonBuffer - Overriding toString #1073

merged 2 commits into from
Sep 29, 2025

Conversation

vijaykriishna
Copy link
Contributor

@vijaykriishna vijaykriishna commented Sep 26, 2025 •
edited
Loading

Ref. #866 Overriding toString function to print readable JSON pulled in from JacksonJsonBuffer.
@vijaykriishna vijaykriishna marked this pull request as ready for review September 26, 2025 01:07
@l-trotta l-trotta self-assigned this Sep 26, 2025
@vijaykriishna vijaykriishna force-pushed the feat/866-JacksonJsonBuffer-toString branch from 56e343d to e4049e9 Compare September 27, 2025 05:42
@l-trotta
Copy link
Contributor

Hey @vijaykriishna thanks for contributing! This looks good, I just have one change suggestion: in the catch, it would be better to rethrow a Runtime exception instead of returning a string. The message can stay the same, just remove the < >. Could you update the PR?
@vijaykriishna
Copy link
Contributor Author

Hey @vijaykriishna thanks for contributing! This looks good, I just have one change suggestion: in the catch, it would be better to rethrow a Runtime exception instead of returning a string. The message can stay the same, just remove the < >. Could you update the PR?

I get your point. In my opinion, throwing RuntimeException can cause unexpected failures and this function I believe is not part of critical flow/ execution path. However, we can log this as error. Thoughts ?
@l-trotta
Copy link
Contributor

@vijaykriishna okay I'm convinced, let's keep it as is! just remove the < > and then it will be ready
@vijaykriishna
Copy link
Contributor Author

@vijaykriishna okay I'm convinced, let's keep it as is! just remove the < > and then it will be ready

Done.
l-trotta
l-trotta approved these changes Sep 29, 2025
View reviewed changes
Copy link
Contributor

@l-trotta l-trotta left a comment

Choose a reason for hiding this comment

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

thanks!
@l-trotta l-trotta merged commit 57ed8f8 into elastic:main Sep 29, 2025
8 checks passed
@l-trotta l-trotta mentioned this pull request Sep 29, 2025
Closed
l-trotta pushed a commit that referenced this pull request Oct 7, 2025
@vijaykriishna @l-trotta
866 - JacksonJsonBuffer - Overriding toString (#1073)
c891db7 
* 866 - JacksonJsonBuffer - Overriding toString * 866 - Formatting error message
l-trotta pushed a commit that referenced this pull request Oct 7, 2025
@vijaykriishna @l-trotta
866 - JacksonJsonBuffer - Overriding toString (#1073)
97cd32d 
* 866 - JacksonJsonBuffer - Overriding toString * 866 - Formatting error message
l-trotta pushed a commit that referenced this pull request Oct 7, 2025
@vijaykriishna @l-trotta
866 - JacksonJsonBuffer - Overriding toString (#1073)
876792b 
* 866 - JacksonJsonBuffer - Overriding toString * 866 - Formatting error message
l-trotta pushed a commit that referenced this pull request Oct 7, 2025
@vijaykriishna @l-trotta
866 - JacksonJsonBuffer - Overriding toString (#1073)
4bfb003 
* 866 - JacksonJsonBuffer - Overriding toString * 866 - Formatting error message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Helpers backport 8.19 backport 9.0 backport 9.1 Category: Enhancement New feature or request

2 participants

@vijaykriishna @l-trotta