-
Notifications
You must be signed in to change notification settings - Fork 24
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
Added 'raw_json' param to api calls. #200
base: master
Are you sure you want to change the base?
Conversation
Fixes issues with encoded URIs and gives way more data with each call.
Param 'raw_json' will be applied, regardless of whether params are already passed or not.
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.
I don't think we can make this change at this particular point in time as it's a breaking change. Not ruling it out for a 2.x.x release though!
Also, you'll want to run the test suite for the package to make sure everything passes (except for the tests under test/auth/
) by running dart test/test_all.dart
.
@@ -620,6 +620,8 @@ class Reddit { | |||
'Cannot make requests using unauthenticated client.'); | |||
} | |||
final path = Uri.https(defaultOAuthApiEndpoint, api); | |||
params ??= {}; | |||
params['raw_json'] = '1'; |
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.
I'm not sure this is something we'll want to do in an incremental release since it could break users relying on the existing behavior, and that's not accounting for the existing test data. Since the params won't match the original requests made to generate the test data, any test which calls get
will fail with this change.
If we're going to change the default, we should bump the major package version to 2.x.x, but we'll need to do more work before we can do that.
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.
Hmm.. The tests might be an issue but as far as i have seen, this doesn't bring any breaking changes. It adds extra fields and removes the & in links(which should be fine as most likely everyone is using .replaceAll() to take care of this).
I am already using a forked version of this repo to add this so its no big problem for me :).
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.
If we're going to change the default, we should bump the major package version to 2.x.x, but we'll need to do more work before we can do that.
Do you have some sort of a road map for this or are you basing this off of PRAW
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.
You're probably right, but we'll want to be consistent and make sure this isn't something we want to pass for POST
and other HTTP request types.
We'll also need to update the tests, which is going to require us to modify the recorded requests in the JSON files. There's probably a good way to find/replace, but we might need to just bite the bullet and parse the JSON, find the requests, and then shove the raw_json=1
parameter in the right place and write them back to file.
If we can get those tests updated and passing, I think we can land this.
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.
We dont need to pass the raw_json
to POST
types because that argument only changes how reddit sends the data. It won't effect other request types.
I dont exactly know how the test json files are written else i could run it through a script or since i am on linux, there are quite good tools built in through which i find and replace really fast.
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.
What if instead of waiting for a major bump not add an optional parameter to the Reddit
class?
I if it null
, we don' add anything to requests. This parameter could then become by default true
in the 2.0
major release bump but would not cause any backwards issues.
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.
What if instead of waiting for a major bump not add an optional parameter to the
I if itnull
, we don' add anything to requests. This parameter could then become by defaulttrue
in the2.0
major release bump but would not cause any backwards issues.
You can add a PR
for this if you want. The actual maintainer of the repo hasn't been active for a while so it might take a while before it gets merged though. I would recommend using a local version of DRAW
with this patch until then.
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.
Yeah, sorry my turn around times aren't super quick. Work has been keeping me busy... :-( I'll see if I can get the tests updated in the next day or so and land a new version with the updated behavior.
Fixes issues with encoded URIs and gives way more data with each call.
Notably the issue with icon images containing '&', making it hard to open images in browser/load them.