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

Simple requests #21

Open
g4s8 opened this issue Apr 4, 2018 · 22 comments
Open

Simple requests #21

g4s8 opened this issue Apr 4, 2018 · 22 comments

Comments

@g4s8
Copy link

g4s8 commented Apr 4, 2018

I think there should be some simple request objects, like:

new HtResponse(
  new RqSimple("GET", "https://foo.bar?param=1")
);
// or
new HtResponse(
  new RqSimple.Get("https://foo.bar?param=1")
);
@0crat
Copy link
Collaborator

0crat commented Apr 4, 2018

@yegor256/z please, pay attention to this issue

@0crat
Copy link
Collaborator

0crat commented Apr 4, 2018

@g4s8/z this project will fix the problem faster if you donate a few dollars to it; just click here and pay via Stripe, it's very fast, convenient and appreciated; thanks a lot!

@krzyk
Copy link
Contributor

krzyk commented May 9, 2018

@llorllale I was actually looking how to do a request and it turns out I need to construct the http request using strings. Having something like here would make it much easier.

@yegor256
Copy link
Owner

@g4s8 I think we should recommend our users to use those request building objects from Takes Framework. Reimplementing them here would be silly, I think.

@krzyk
Copy link
Contributor

krzyk commented May 10, 2018

@llorllale so this could be about extending the documentation (README.md) with example usage (replacing the JoinedText example with something less bare bones and more abstract).

@g4s8
Copy link
Author

g4s8 commented May 10, 2018

@yegor256 you meant that cactoos-http should depends on takes? I think it's not good, because it can be used not in web apps only or with other web-frameworks.

@yegor256
Copy link
Owner

@g4s8 no, it shouldn't depend on Takes. But, as @krzyk suggested, we should say in our README that: "if you need a more powerful instrument to build those HTTP requests, use Takes Framework, for example: ..."

@llorllale
Copy link
Collaborator

@yegor256 why isn't Takes included in the list of competitors?

@yegor256
Copy link
Owner

@llorllale because it doesn't know how to make HTTP requests. It can only build them in text form.

@llorllale
Copy link
Collaborator

@yegor256 I think you meant jcabi-http when you said:

I think we should recommend our users to use those request building objects from Takes Framework. Reimplementing them here would be silly, I think.

But I believe the design of jcabi-http is exactly what we're trying to avoid in cactoos-http.

@yegor256
Copy link
Owner

@llorllale I meant Takes, not jcabi-http

@llorllale
Copy link
Collaborator

@g4s8 @yegor256 @krzyk it would sound really weird for cactoos-http to direct users to another lib/framework just to fulfill this simple requirement (build http requests).

@llorllale
Copy link
Collaborator

@yegor256

we should recommend our users to use those request building objects from Takes Framework. Reimplementing them here would be silly,

I think then Takes should depend on cactoos-http

@yegor256
Copy link
Owner

@llorllale well, not a bad idea, but will require a lot of refactoring and class moving...

@llorllale
Copy link
Collaborator

@yegor256 Takes has the option of just continuing doing their own thing.

@llorllale
Copy link
Collaborator

@g4s8 why did you leave out Wire in your proposal?

@llorllale
Copy link
Collaborator

@yegor256 @g4s8 tell me what you think:

new HtResponse(
    new HtWire("host", port),
    new RqHeaders(
        new IterableOf<>(
            "Host: host:port",
            "Accept: */*"
        ),
        new RqBase(
            "GET", "/some/resource",
        )
    )
)

All requests would still be of type org.cactoos.Input.

With these decorators, we will basically be splicing and joining several Input or InputStream into a single one.

What we would need:

  • An equivalent for java's SequenceInputStream (org.cactoos.io.JoinedInput that implements Input)
  • SkipInput (I'm tempted to rename to something else)
  • The opposite of SkipInput

Thoughts
Insisting on keeping the Input abstraction on requests sure makes implementing decorators more difficult.

@yegor256
Copy link
Owner

yegor256 commented Jul 3, 2018

@llorllale I like the code snippet above, but I didn't understand the text beneath. We already have everything that we need for this code. What else is needed and what is your question?

@llorllale
Copy link
Collaborator

llorllale commented Jul 3, 2018

@yegor256 @g4s8

In my example above, we have two requests - RqBase and RqHeaders - and both are of type Input.

A Wire accepts only one Input in its method: Input send(Input request);.

Therefore we need to join all those requests/inputs into a single one and send it via the wire.

My question was if you guys accept this solution?

@yegor256
Copy link
Owner

@llorllale I didn't understand this:

In my example above, we have two requests - RqBase and RqHeaders - and both are of type Input.

RqBase (from Takes) is of time Request. I'm lost.

@llorllale
Copy link
Collaborator

@yegor256 the requirement is for providing "simple requests", not necessarily copy request objects from Takes.

Here is my example again with the Htr prefix to avoid confusion:

new HtResponse(
    new HtWire("host", port),
    new HtrHeaders(
        new IterableOf<>(
            "Host: host:port",
            "Accept: */*"
        ),
        new HtrBase(
            "GET", "/some/resource",
        )
    )
)

This Htr prefix is only valid for the purposes of this example, as I have yet to come up with a better prefix. (I think there would be too many naming conflicts if we use Ht for these)

@victornoel
Copy link
Contributor

@g4s8 @llorllale @yegor256 for the record, there are such simple request objects already emerging in the test code base of cactoos-http, see #79. Just noting that here so that they are merged with whatever solution is found in this issue :)

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

6 participants