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

Add color to request and response body when content-type is json #930

Merged
merged 24 commits into from
Mar 4, 2023

Conversation

Amirhy
Copy link
Contributor

@Amirhy Amirhy commented Nov 28, 2022

📷 Screenshots

Screen Shot 1401-09-07 at 14 21 45

Screen Shot 1401-09-07 at 14 22 09

📄 Context

I have been using chucker for a long time. But the gray texts in the body of the requests and responses bothered me and my teammates's eyes and also made it difficult for us to read the texts properly.
So I decided to color the text of body of requests and responses.
Although I've only done it for application/json content-type and I'll support the rest of them soon.
I also fixed a bug that if there is no HeaderItem type inside the adapter items and you search for a text in the body of requests and responses, the text does not highlight properly.

📝 Changes

  • Implement colorful text for body of request and response when content-type is application/json

🚫 Breaking

🛠️ How to test

I just write some tests for json formatting in SpanUtilTest class that you can run them and also you can do functional testing.

⏱️ Next steps

Implement this feature for other content-types , for example application/xml

@Amirhy Amirhy changed the title Feat colorful body Add color to request and response body when content-type is json Nov 28, 2022
@cortinico
Copy link
Member

Thanks for sending this @Amirhy. I won't be able to review this till sometime next week as I'm a bit busy at the moment 👍

@Amirhy
Copy link
Contributor Author

Amirhy commented Dec 11, 2022

Hi @cortinico. Hope you 're doing well.
Did you have free time to review my PR?

@cortinico
Copy link
Member

Did you have free time to review my PR?

Sorry I was quite busy this week. I'll have to look into this over the next weekend.
In the meanwhile @ArjanSM do you want to do a pass here?

@ArjanSM
Copy link
Contributor

ArjanSM commented Dec 14, 2022

Did you have free time to review my PR?

Sorry I was quite busy this week. I'll have to look into this over the next weekend. In the meanwhile @ArjanSM do you want to do a pass here?

@cortinico I've been a little occupied today. I'll take a look at it tomorrow.

Copy link
Contributor

@ArjanSM ArjanSM left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Amirhy 🏅
I've made a few suggestions.
Also, I personally feel that it would be nice if you could create different PRs (one for text coloring feature and the other for the Text highlighting bug) or the reference to the feature issue and the bug in the PR description.

@Amirhy
Copy link
Contributor Author

Amirhy commented Dec 24, 2022

Thanks for the PR @Amirhy 🏅 I've made a few suggestions. Also, I personally feel that it would be nice if you could create different PRs (one for text coloring feature and the other for the Text highlighting bug) or the reference to the feature issue and the bug in the PR description.

Thanks for your time @ArjanSM. I will fix them as soon as possible.

@Amirhy
Copy link
Contributor Author

Amirhy commented Dec 25, 2022

Thanks for the PR @Amirhy 🏅 I've made a few suggestions. Also, I personally feel that it would be nice if you could create different PRs (one for text coloring feature and the other for the Text highlighting bug) or the reference to the feature issue and the bug in the PR description.

Ok I removed my changes that fix the mentioned bug, and I will create another PR for them.

@Amirhy
Copy link
Contributor Author

Amirhy commented Jan 4, 2023

@cortinico @ArjanSM kindly reminder

Copy link
Contributor

@ArjanSM ArjanSM left a comment

Choose a reason for hiding this comment

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

sorry for the delay. Just a few to nits to begin with. I'll give it a more thorough pass later in the week.

@Amirhy Amirhy closed this Feb 7, 2023
@Amirhy Amirhy reopened this Feb 7, 2023
@Amirhy
Copy link
Contributor Author

Amirhy commented Feb 7, 2023

sorry for the delay. Just a few to nits to begin with. I'll give it a more thorough pass later in the week.

Hi @ArjanSM,

Hope you are doing well.

I resolved all threads you created.Please let me know if you have any other comment.

Copy link
Contributor

@ArjanSM ArjanSM left a comment

Choose a reason for hiding this comment

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

Getting there!
Just a bunch of few nits.

@Amirhy
Copy link
Contributor Author

Amirhy commented Feb 22, 2023

Hi Dear @ArjanSM

Do you have any new nits about my changes? ;-)

I want you check this comment and let me know what is your opinion?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Code looks good to me 👍 @ArjanSM already did a great review!
Only left a minor nits.

Comment on lines +328 to +329
if (result.isEmpty())
result.add(subSequence(0, length))
Copy link
Member

Choose a reason for hiding this comment

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

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because when the char sequence doesn't have any lines so I should add all of its content to the result.

@Amirhy Amirhy requested a review from a team as a code owner February 28, 2023 08:17
@ArjanSM
Copy link
Contributor

ArjanSM commented Mar 3, 2023

Hi Dear @ArjanSM

Do you have any new nits about my changes? ;-)

I want you check this comment and let me know what is your opinion?

Aah! my bad! 👍

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

This is awesome @Amirhy Thanks for taking the time to add this feature!

@cortinico cortinico merged commit 3ae4d08 into ChuckerTeam:develop Mar 4, 2023
@misael-jonathan
Copy link

hi @Amirhy , I think there's performance issue when we used this implementation in large json data. I just updated my chucker with this colored request/response, in some cases it took 2-3 secs to load my response, and if its super big it'll just stuck on loading state. Can u verify this issue on your side?

@Amirhy
Copy link
Contributor Author

Amirhy commented Apr 16, 2023

Hi @misael-jonathan ,
Thanks for sharing your experience.
If it is possible please send me your json to my email ([email protected])
I will investigate on it and inform you as soon as possible.

@misael-jonathan
Copy link

Hi, I just sent the json to your email. Actually it is the same json you can find in LargeJson.kt in the sample app.
So, I think you can start from there.

Thanks @Amirhy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants