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

vibe.web.web: add convenience methods for status and header #1696

Merged
merged 2 commits into from Jul 5, 2017

Conversation

Projects
None yet
2 participants
@wilzbach
Contributor

wilzbach commented Feb 27, 2017

These methods are pretty trivial, but convenient in practice.

Show outdated Hide outdated web/vibe/web/web.d
Note that this may only be called from a function/method
registered using registerWebInterface.
*/
@property void status(int statusCode)

This comment has been minimized.

@s-ludwig

s-ludwig Mar 25, 2017

Member

I think property syntax may be a little surprising here (not that omitting @property would change anything, but I'd not endorse the status = x; syntax).

@s-ludwig

s-ludwig Mar 25, 2017

Member

I think property syntax may be a little surprising here (not that omitting @property would change anything, but I'd not endorse the status = x; syntax).

This comment has been minimized.

@wilzbach

wilzbach Jun 29, 2017

Contributor

Yeah I agree, but 201.status is just awesome ;-)

@wilzbach

wilzbach Jun 29, 2017

Contributor

Yeah I agree, but 201.status is just awesome ;-)

This comment has been minimized.

@s-ludwig

s-ludwig Jun 29, 2017

Member

I'm not quite sure I'd agree there 😄 ...but that would actually be UFCS, not @property.

@s-ludwig

s-ludwig Jun 29, 2017

Member

I'm not quite sure I'd agree there 😄 ...but that would actually be UFCS, not @property.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 25, 2017

Member

Makes sense. I'd also add a short usage example to each declaration, e.g.:

///
unittest {
    class WebService {
        Json postCreateItem() {
            // ...
            status(HTTPStatus.created);
            return Json(["id": 100]);
        }
    }
}
Member

s-ludwig commented Mar 25, 2017

Makes sense. I'd also add a short usage example to each declaration, e.g.:

///
unittest {
    class WebService {
        Json postCreateItem() {
            // ...
            status(HTTPStatus.created);
            return Json(["id": 100]);
        }
    }
}
@wilzbach

This comment has been minimized.

Show comment
Hide comment
@wilzbach

wilzbach Jun 29, 2017

Contributor

Makes sense. I'd also add a short usage example to each declaration, e.g.:

Alright while I was at it, I also added an example for the already existing terminateSession and redirect.

(sorry for the rebase spam - I discovered a couple of nits).

Contributor

wilzbach commented Jun 29, 2017

Makes sense. I'd also add a short usage example to each declaration, e.g.:

Alright while I was at it, I also added an example for the already existing terminateSession and redirect.

(sorry for the rebase spam - I discovered a couple of nits).

@wilzbach

This comment has been minimized.

Show comment
Hide comment
@wilzbach

wilzbach Jun 29, 2017

Contributor

that omitting @Property would change anything

Done. Btw have you considered doing sth. like TaskLocal!RequestContext s_requestContext for the new, upcoming http module? Not needing to specify req and res in the parameters, is very handy - though I am not sure how much overhead TaskLocal would bring.

Contributor

wilzbach commented Jun 29, 2017

that omitting @Property would change anything

Done. Btw have you considered doing sth. like TaskLocal!RequestContext s_requestContext for the new, upcoming http module? Not needing to specify req and res in the parameters, is very handy - though I am not sure how much overhead TaskLocal would bring.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 29, 2017

Member

I was thinking more about a way to possibly make the request/response handling more safe w.r.t. order of operations and possibly more efficient at the same time (by avoiding allocations for example). Unfortunately, D doesn't have anything particularly useful for this in its type system, though, so I'm not sure how far that will get.

For the convenience part, IMO, having the web module should be sufficient, as I'd expect all usual applications to be written in terms of vibe.web.* nowadays. So if anywhere, I'd expose it there. But then again, the goal of that module is to hide the bare request/response as well as possible by providing more robust high-level logic, so the question is if this would send the right message.

Performance-wise it shouldn't be critical unless it's queried numerous times per request. It's not exactly free, but definitely dwarfed by everything that happens in the request/response processing logic.

Member

s-ludwig commented Jun 29, 2017

I was thinking more about a way to possibly make the request/response handling more safe w.r.t. order of operations and possibly more efficient at the same time (by avoiding allocations for example). Unfortunately, D doesn't have anything particularly useful for this in its type system, though, so I'm not sure how far that will get.

For the convenience part, IMO, having the web module should be sufficient, as I'd expect all usual applications to be written in terms of vibe.web.* nowadays. So if anywhere, I'd expose it there. But then again, the goal of that module is to hide the bare request/response as well as possible by providing more robust high-level logic, so the question is if this would send the right message.

Performance-wise it shouldn't be critical unless it's queried numerous times per request. It's not exactly free, but definitely dwarfed by everything that happens in the request/response processing logic.

Show outdated Hide outdated web/vibe/web/web.d
// POST /item
Json postItem() {
header("X-RateLimit-Remaining", 59);
return Json(["id": 100]);

This comment has been minimized.

@s-ludwig

s-ludwig Jun 30, 2017

Member
web/vibe/web/web.d(428,10): Error: function vibe.web.web.header (string name, string value) is not callable using argument types (string, int)
web/vibe/web/web.d(429,15): Error: none of the overloads of '__ctor' are callable using argument types (int[string]), candidates
@s-ludwig

s-ludwig Jun 30, 2017

Member
web/vibe/web/web.d(428,10): Error: function vibe.web.web.header (string name, string value) is not callable using argument types (string, int)
web/vibe/web/web.d(429,15): Error: none of the overloads of '__ctor' are callable using argument types (int[string]), candidates
Show outdated Hide outdated web/vibe/web/web.d
// POST /item
Json postItem() {
status(HTTPStatus.created);
return Json(["id": 100]);

This comment has been minimized.

@s-ludwig

s-ludwig Jun 30, 2017

Member

Also needs -> Json(100)

@s-ludwig

s-ludwig Jun 30, 2017

Member

Also needs -> Json(100)

@wilzbach

This comment has been minimized.

Show comment
Hide comment
@wilzbach

wilzbach Jun 30, 2017

Contributor

Also needs

Damn, sorry. I just ran dub test locally and forgot that I have to run dub test :web - is there an easy way that we could make dub test run test on all sub packages by default?

Contributor

wilzbach commented Jun 30, 2017

Also needs

Damn, sorry. I just ran dub test locally and forgot that I have to run dub test :web - is there an easy way that we could make dub test run test on all sub packages by default?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 5, 2017

Member

is there an easy way that we could make dub test run test on all sub packages by default?

We should definitely do that. Of course using "--combined" also works, but ideally dub test would be all that one has to run. I'd even like to implement a way to integrate high-level test scripts.

Member

s-ludwig commented Jul 5, 2017

is there an easy way that we could make dub test run test on all sub packages by default?

We should definitely do that. Of course using "--combined" also works, but ideally dub test would be all that one has to run. I'd even like to implement a way to integrate high-level test scripts.

@s-ludwig s-ludwig merged commit 52d1b3d into vibe-d:master Jul 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilzbach wilzbach deleted the wilzbach:header-and-status branch Jul 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment