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.

Sign up for GitHub

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

Jump to bottom

Added 'raw_json' param to api calls. #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added 'raw_json' param to api calls. #200

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e31953c
Added 'raw_json' param to api calls.
SupremeDeity Jul 27, 2021
a0f0f05
Param 'raw_json' un-overridable.
SupremeDeity Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/src/reddit.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,8 @@ class Reddit {
'Cannot make requests using unauthenticated client.');
}
final path = Uri.https(defaultOAuthApiEndpoint, api);
params ??= {};
params['raw_json'] = '1';
Copy link
Member

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.

Copy link
Contributor Author

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 :).

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Contributor Author

@SupremeDeity SupremeDeity Sep 19, 2021 •
edited
Loading

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.

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.

Copy link
Member

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.

final response =
await auth.get(path, params: params, followRedirects: followRedirects);
return objectify ? _objector.objectify(response) : response;
Expand Down