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

Remove '?' prefix in FeedOption::build_url #88

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

Jaltaire
Copy link
Contributor

@Jaltaire Jaltaire commented Jun 1, 2024

Repeated inclusion of "?" to identify the query string will cause any parameters to which "?" is appended to be treated as "param?". This leads to the parameter being ignored by the Reddit API as it cannot be parsed correctly.
For example, when calling Subreddit::new("ipad").latest(1, Some(FeedOption::new()), the URL will be formatted as "https://www.reddit.com/r/Watchexchange/new.json?limit=1?", and limit=1 is ignored.

For now, let's remove the "?" appending in FeedOption::build_url and instead include it in every URL passed to build_url.

Ultimately, we should probably rethink how we're specifying options. Current design, for example, allows limit to be specified twice in many places. The second limit specified in the URL will always be the one used (overriding the first).

Likewise, the splitting of options between FeedOptions and as function parameters makes for a confusing API. If FeedOptions are truly applicable everywhere, then we should remove redundant provisions of limit. If certain options (e.g., before and after) are only applicable in certain places, then perhaps we can forgo FeedOptions and provide the appropriate parameters for each function corresponding to a REST API call.

Repeated inclusion of "?" to identify the query string will cause
any parameters to which "?" is appended to be treated as "param?".
This leads to the parameter being ignored by the Reddit API as it
cannot be parsed correctly.

For now, let's remove the "?" appending in FeedOption::build_url
and instead include it in every URL passed to build_url.
@Lurchfresser
Copy link

Noticed the same issue, your pull request seems to fix the issue, even though I didnt test it. Also agreeing on the FeedOptions take.

@beanpuppy
Copy link
Collaborator

Agreed @Jaltaire. However, I'm not particularly intertested in maintaining this library anymore (I've wanted to make an issue looking for new maintainers for a while) so I doubt I'll put much work to it, but I'd be happy to merge any new design you come up with if you wanted.

@beanpuppy beanpuppy merged commit e4eed92 into halcyonnouveau:master Jul 17, 2024
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.

3 participants