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

Response processing refactor #554

Merged
merged 3 commits into from
Feb 10, 2021
Merged

Conversation

MiSikora
Copy link
Contributor

@MiSikora MiSikora commented Feb 9, 2021

📄 Context

This is the final PR that is a result of splitting #527.

📝 Changes

Similarly to the RequestProcessor class I added the ResponseProcessor class that does the analogous stuff. I changed one thing, where plain text body is set to be plainText = true only when there is some content. I don't think it made sense to set it to true when there was no body.

📎 Related PR

#527

🚫 Breaking

No.

🛠️ How to test

Rely on CI. Manually you can simply check the sample app.

⏱️ Next steps

N/A

@cortinico cortinico added this to the 4.0.0 milestone Feb 9, 2021
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.

LGTM, just a minor comment

@MiSikora
Copy link
Contributor Author

MiSikora commented Feb 9, 2021

Bumping because I don't want to get this message lost in a resolved comment. :)

@cortinico Ok, I pushed the change in 007cd55 without the init block. Let me know which one you prefer more. I'm ok either way.

@cortinico
Copy link
Member

@cortinico Ok, I pushed the change in 007cd55 without the init block. Let me know which one you prefer more. I'm ok either way.

I'd go for the solution without the init{} block as it's more declarative and easier to follow :)

@MiSikora MiSikora merged commit 5420645 into develop Feb 10, 2021
@MiSikora MiSikora deleted the response-processing-refactor branch February 10, 2021 15:58
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.

3 participants