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

QJsonDocument: Avoid QJsonObject and QJsonArray in interface #24

Open
wants to merge 2 commits into
base: QTTPv1.0.0
Choose a base branch
from

Conversation

lennartS-7cs
Copy link

Previously QJsonObject was used to pass response data. That made it
impossible to pass an array as data to the client. To allow both
single objects and whole lists of objects, QJsonDocument is now used,
which is capable of containing both an object and an array.

Signed-off-by: Lennart Sauerbeck lennart.sauerbeck@sevencs.com

Previously QJsonObject was used to pass response data. That made it
impossible to pass an array as data to the client. To allow both
single objects and whole lists of objects, QJsonDocument is now used,
which is capable of containing both an object and an array.

Signed-off-by: Lennart Sauerbeck <lennart.sauerbeck@sevencs.com>
@supamii
Copy link
Owner

supamii commented Aug 29, 2017

Thanks for the pull request!

@@ -38,7 +38,7 @@ HttpResponse& HttpData::getResponse()
return m_HttpResponse;
}

void HttpData::setResponse(const QJsonObject& json)
void HttpData::setResponse(const QJsonDocument& json)
Copy link
Owner

@supamii supamii Aug 30, 2017

Choose a reason for hiding this comment

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

I went ahead and tried against Qt 5.7 and this will throw up.

/Users/dev/personal/qttpPR/test/qttptest/qttptest.h:23: error: non-const lvalue reference to type 'QJsonObject' cannot bind to a value of unrelated type 'QJsonDocument' QJsonObject& json = data.getResponse().getJson(); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

As a compromise, perhaps a MACRO can toggle between the types - however the cascading problem here will be that the object assignments will also be affected.

I am installing 5.9 now and I assume it'll run without a hitch.

Did I do something wrong? Your thoughts on how to proceed?

Copy link
Owner

Choose a reason for hiding this comment

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

So looks like we need to update test files :)

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry. I had trouble getting my local Qt version with MSVC17 to work in QtCreator. That is why I didn't build the tests.

I'll take a look at them right away.

Signed-off-by: Lennart Sauerbeck <lennart.sauerbeck@sevencs.com>
@lennartS-7cs
Copy link
Author

lennartS-7cs commented Aug 31, 2017

After 41a896a the tests build again but four of them fail as they did before my changes. I'll create a new commit fixing those and add them to this pull request. Nevermind, I had a look at it and don't quite get what is happening there. It probably should be in a separate pull request.

Please allow me to squash the two commits into one before accepting the pull request once you're okay with my changes.

@lennartS-7cs
Copy link
Author

lennartS-7cs commented Aug 31, 2017

Oh, and before I forget: I made this change while working for my employer. Do you need anything (besides the Signed-Off-By clause) to be okay with corporate changes into your code? We're prepared to send you an e-mail from my manager which wuold grant me the right to publish this change under the MIT license.

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.

2 participants