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

Redesign: Separate data structures from HTTP client #8

Closed
DoumanAsh opened this issue Oct 2, 2018 · 8 comments
Closed

Redesign: Separate data structures from HTTP client #8

DoumanAsh opened this issue Oct 2, 2018 · 8 comments

Comments

@DoumanAsh
Copy link
Contributor

DoumanAsh commented Oct 2, 2018

Currently, API is tied to reqwest as HTTP client so anyone who would want to use own HTTP Client would be unable to do it as Pixiv and PixivRequestBuilder are tied to it.

So what would be better if we could use
PixivRequestBuilder to create PixivRequest

And then user would need to pass PixivRequest to Pixiv for execution.

Pixiv itself would be in a separate module and optional, only if user would not disable default feature that enables reqwest

Most likely it would require to rename it.

Most likely ready to be send PixivRequest would its field to be public so that user could create request from it.
As most existing HTTP clients use http crate for basic primitives it should be easy to use it with any other clients

P.s. Well I'd like to have this separation because I prefer my own HTTP client to reqwest

@fairingrey
Copy link
Owner

fairingrey commented Oct 2, 2018

That makes sense! I didn't think of it that way when I first made this crate. (Honestly I just wanted something to work with while creating a chatbot, so I just searched whatever was the most convenient HTTP client library to use, and reqwest popped up)

If I'm understanding this correctly, we just need to make the Pixiv component optional (and that would probably use reqwest by itself), but PixivRequest would be able to generate or convert itself to a http response object (this, I'm assuming) if users wanted to use their own http client, right? (Whether that be reqwest, hyper, or something else.) Hopefully I'm in the same vein of understanding here.

Would you like to work on it and propose another pull request for it? I'd be willing to try and help, though I haven't touched Rust in a while and I'm a little unfamiliar with how site-specific, unofficial API libraries should actually be designed in practice here.

Another concern I should share is that I'm blissfully unaware of how the versioning would change given a rather dramatic library redesign. Though perhaps you may know more about that than I do, so do let me know if at all possible!

@DoumanAsh
Copy link
Contributor Author

DoumanAsh commented Oct 2, 2018

Yes, ideally it would be the best to convert to http::Request but it requires you specialize it with Body, which may not be always desirable.
So I'm not sure yet how to handle it.

If I'm understanding this correctly, we just need to make the Pixiv component optional

Yes, it would imply that user would use request builder, then somehow create request for his library of choice.
I'm inclined to use http primitives in PixivRequest so that user could use them to generate request himself, instead of library providing it for him since as I mentioned http::Request is generic

Would you like to work on it and propose another pull request for it?

Yes, since I'd like to use library instead of writing all the code myself, I'm planning to work on it.
As I don't use reqwest and my own library

Another concern I should share is that I'm blissfully unaware of how the versioning would change given a rather dramatic library redesign.

In Rust world the basic rules are:

  • Before 1.0.0: 0.x.y - x is incremented on breaking changes
  • After 1.0.0: a.x.y - a is incremented on breaking changes

@fairingrey
Copy link
Owner

Yeah, I don't think we ever use the body in any of the requests made here. I don't know if it would cause a problem, but I could try playing around with it, see what's valid or not.

I gotcha. Looking forward to it! And I'll have to check out yukikaze, it seems interesting!

@DoumanAsh
Copy link
Contributor Author

Could you please re-open issue? 😄

@fairingrey fairingrey reopened this Oct 8, 2018
@fairingrey
Copy link
Owner

Whoops, my bad. There you go.

@DoumanAsh
Copy link
Contributor Author

As for current issue.
When pixiv builds request it stores url where to send it
And as of now it uses request::Url with that we have two options:

  1. Store it as String and convert in client
  2. Use common http library and its Uri struct to represent address. As of now reqwest client would still require conversion

@fairingrey
Copy link
Owner

I think I'd like doing it the second way more. It seems like a more idiomatic design, I think.

@DoumanAsh
Copy link
Contributor Author

Yeah, let's go with it I guess.
It would be easier to convert components of http request to actual request for library as most of them rely on http

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

No branches or pull requests

2 participants