Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Conversation

@igorpeshansky
Copy link
Contributor

No description provided.

@igorpeshansky igorpeshansky force-pushed the igorp-named-api-handlers branch 3 times, most recently from 77644ee to 146bdd5 Compare March 28, 2018 22:26
Copy link
Contributor

@supriyagarg supriyagarg left a comment

Choose a reason for hiding this comment

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

LGTM

: handler_(config, store),
: config_(config), store_(store), dispatcher_({
{{"GET", "/monitoredResource/"},
[=](const HttpServer::request& request,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to just pass HandleMonitoredResource, without doing so in a lambda? This would work in Javascript, but I know C++ is much stricter than javascript, I think anything is stricter than Javascript :-P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because we also need to capture this. If there were no capture, you could indeed convert a function pointer into an std::function.

if (config_.VerboseLogging()) {
LOG(INFO) << "Found resource for " << id << ": " << resource;
}
conn->set_status(HttpServer::connection::ok);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you rebase off of master with the latest header updates and 404 response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.

@igorpeshansky igorpeshansky force-pushed the igorp-named-api-handlers branch from 146bdd5 to b1376e0 Compare March 30, 2018 06:31
Copy link
Contributor Author

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

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

PTAL.

: handler_(config, store),
: config_(config), store_(store), dispatcher_({
{{"GET", "/monitoredResource/"},
[=](const HttpServer::request& request,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because we also need to capture this. If there were no capture, you could indeed convert a function pointer into an std::function.

if (config_.VerboseLogging()) {
LOG(INFO) << "Found resource for " << id << ": " << resource;
}
conn->set_status(HttpServer::connection::ok);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done.

Copy link
Contributor

@bmoyles0117 bmoyles0117 left a comment

Choose a reason for hiding this comment

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

LGTM

@igorpeshansky igorpeshansky merged commit 5538e25 into master Mar 30, 2018
@igorpeshansky igorpeshansky deleted the igorp-named-api-handlers branch March 30, 2018 13:36
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.

3 participants