-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
Refactor/Cleanup NetworkRequest and Related Code #4633
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpicks, otherwise looks good
1df8848
to
d886b41
Compare
Constructing the url-query and json in Helix could be simplified by using the constructors taking initializer-lists. |
d886b41
to
df1e012
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes look good to me 👍
If you want to re-add the assert comments in any way feel free, will merge this in later
Description
This PR does many small changes - each commit can be reviewed on its own.
The following changes were made:
json
methods that simplify sending JSON. This also sets theAccepts
header, since here, when JSON is sent, only JSON is accepted as a response (this is only done in Helix).Helix
'smakeRequest
now accepts an HTTP method.Network{Request,Private}
. The only suggestions left there are (1) aconst-cast
and (2-5) unusedQFuture
's fromQtConcurrent::run
calls. (1) should be resolved when Remove QObjectRef in favor of QPointer #4632 is closed. (2-5) can be resolved by usingQThreadPool::start
, since we don't need the future.asserts
toQ_ASSERT_X
, which provides better error messages (mainly with a reasoning in the message).