Skip to content
This repository has been archived by the owner on Mar 23, 2020. It is now read-only.

RFC: Allow since_id to be specified to get statuses #7

Closed
wants to merge 2 commits into from

Conversation

wezm
Copy link

@wezm wezm commented Jun 13, 2017

Initial support for passing since_id to Mastodon::statuses. I need this for a tool I'm working on. It gets the job done for this task but since several endpoints allow since_id to be specified it might be worth making this into a builder. I note there's also a TODO about allowing limit to be specified via the builder pattern too.

@@ -14,6 +14,7 @@ reqwest = "0.6"
serde = "1"
serde_json = "1"
serde_derive = "1"
url = "1.5"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url was already a dependency of reqwest, just exposing it to this crate here.

@XAMPPRocky
Copy link
Owner

Hello, and thank you for the PR! I don't agree with this implementation however.

  • Into<Option<T>> is not a good way of specifying optional arguments, it will cause more code generation.
  • While this solves the problem of since_id for GET /statuses it does not solve it for max_id for that route, and doesn't add it to the rest of the other routes.

I feel like the most idiomatic, and easy approach would be to create another builder struct that supplants the Mastodon struct.

struct IdBuilder<'a> {
   mastodon: &'a Mastodon,
   since_id: Option<i64>,
   max_id: Option<i64>,
}

impl<'a> IdBuilder<'a> {
   pub fn statuses(self, id: u64, only_media: bool, exclude_replies: bool) {
       // implementation
   }
}

@wezm
Copy link
Author

wezm commented Jul 3, 2017

I agree your more general solution would be better. I disagree with it not being idiomatic though. The feature was added for this very use: rust-lang/rust#34828 Also I don't think anyone would be calling this method so frequently that any additional code gen would pose significant overhead.

Anyway, this serves my purposes for now. Feel free to close and next time I'm doing some Mastodon work I might pick it back up and look into the general solution.

@XAMPPRocky
Copy link
Owner

Closed now with the availability of Page in 0.10.0-rc2.

@XAMPPRocky XAMPPRocky closed this Feb 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants