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.

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

Newline added to JSON return format (For issue 2676) #5073

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

Conversation

mahajanhrishikesh
Copy link

@mahajanhrishikesh mahajanhrishikesh commented Dec 17, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number: 2676

Description:

Code was not adding a newline to the end of the generated JSON from folly

How do you solve it?

Changed line 61 in GetStatsHandler.cpp from
std::string body = returnJson_ ? folly::toPrettyJson(vals) : toStr(vals);
to
std::string body = returnJson_ ? std::strcat(folly::toPrettyJson(vals), "\n") : toStr(vals);

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

Added an enhancement

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2022

CLA assistant check
All committers have signed the CLA.

@yixinglu yixinglu added the ready-for-testing PR: ready for the CI test label Dec 19, 2022
@Sophie-Xie
Copy link
Contributor

@mahajanhrishikesh Thanks for contribution! Can you fix the lint? Thanks :)

@mahajanhrishikesh
Copy link
Author

@mahajanhrishikesh Thanks for contribution! Can you fix the lint? Thanks :)

Yes! I have to switch it to use snprintf instead of strcat, I will do it soon!

@QingZ11 QingZ11 added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Dec 25, 2023
@QingZ11
Copy link
Contributor

QingZ11 commented Dec 25, 2023

@mahajanhrishikesh Thanks for contribution! Can you fix the lint? Thanks :)

Yes! I have to switch it to use snprintf instead of strcat, I will do it soon!

Long time no see! Are you still working on it? And Merry Christmas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants