Skip to content

QJsonDocument: Avoid QJsonObject and QJsonArray in interface#24

Open
lennartS-7cs wants to merge 2 commits intosupamii:QTTPv1.0.0from
lennartS-7cs:jsondocument
Open

QJsonDocument: Avoid QJsonObject and QJsonArray in interface#24
lennartS-7cs wants to merge 2 commits intosupamii:QTTPv1.0.0from
lennartS-7cs:jsondocument

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!

}

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