Add basic example to page content renderer #39
Conversation
| @@ -25,6 +25,7 @@ deadpool = { version = "0.5", features = ["unmanaged"] } | |||
| deepwell-core = { path = "../deepwell/deepwell-core" } | |||
| deepwell-rpc = { path = "../deepwell-rpc" } | |||
| dns-lookup = "1" | |||
| ftml = { path = "../ftml" } | |||
There was a problem hiding this comment.
You'll need to set travis to download it in order for builds to pass.
src/route/macros.rs
Outdated
| @@ -23,14 +23,27 @@ macro_rules! try_io { | |||
| match $result { | |||
| Ok(object) => object, | |||
| Err(error) => { | |||
| let error = Error::ServiceTransport(error).to_sendable(); | |||
| let error = deepwell_core::error::Error::ServiceTransport(error).to_sendable(); | |||
There was a problem hiding this comment.
N/B alternatively:
| let error = deepwell_core::error::Error::ServiceTransport(error).to_sendable(); | |
| use deepwell_core::error::Error; | |
| let error = Error::ServiceTransport(error).to_sendable(); |
src/route/page.rs
Outdated
| wiki_id: WikiId, | ||
| slug: &str, | ||
| deepwell: &web::Data<DeepwellPool>, | ||
| ) -> std::result::Result<HttpResponse, ()> { |
There was a problem hiding this comment.
use StdResult instead of std::result::Result
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is this necessary? (see below in its usage)
src/route/page.rs
Outdated
| ) -> HttpResponse { | ||
| match get_deepwell_page(wiki_id, slug, &deepwell).await { | ||
| Ok(o) => o, | ||
| Err(()) => get_deepwell_page(wiki_id, "_404", &deepwell).await.unwrap(), // todo: make this safer |
There was a problem hiding this comment.
Agreed this needs to be a helper function. It's actually more complicated: it needs to return the _404 page for the requested page category.
There was a problem hiding this comment.
How do I pass the categories along to deepwell?
src/route/page.rs
Outdated
|
|
||
| Ok(HttpResponse::Ok().json(buffer)) | ||
| } | ||
| Ok(None) => Err(()), |
There was a problem hiding this comment.
Why are we returning an empty error? This case is if there's no page, we should return an Option not Err(()), it's confusing with the fact that there's an actual error case being handled here.
imo it should return Option<HttpResponse>, where None means no such page.
| match try_io_result!(result) { | ||
| Ok(Some(page)) => { | ||
| // now run FTML on it | ||
| let mut contents = match String::from_utf8(page.to_vec()) { |
There was a problem hiding this comment.
Hmm, we'll need to check if it's safe to switch from Box<[u8]> to String. I'll check if wikidot has any non-UTF-8 pages.
src/route/page.rs
Outdated
| buffer.push_str(&output.html); | ||
| buffer.push_str("</body></html>\n"); | ||
|
|
||
| Ok(HttpResponse::Ok().json(buffer)) |
There was a problem hiding this comment.
Returning JSON of a string seems pretty unnecessary. Either we return the string itself or we return the usual output from an API call output.
And yes I agree this is very ad-hoc implementation-wise
| slug: "", | ||
| categories: Vec::new(), | ||
| arguments: HashMap::new(), | ||
| }; | ||
|
|
||
| // TODO get page request | ||
| Ok(HttpResponse::NotImplemented().finish()) | ||
| Ok(get_deepwell_page_wrapped(*TEMP_WIKI_ID, page_req.slug, deepwell).await) |
There was a problem hiding this comment.
Slug needs to changed from "", which is invalid. We need to fetch what the root page is based on wiki settings.
src/route/page.rs
Outdated
| Ok(o) => o, | ||
| Err(()) => get_deepwell_page(wiki_id, "_404", &deepwell).await.unwrap(), // todo: make this safer | ||
| Some(o) => o, | ||
| None => get_deepwell_page(wiki_id, "_404", &deepwell).await.unwrap(), // todo: make this safer |
There was a problem hiding this comment.
In cases where you have
match item {
Some(x) => x,
None => something_else(n),
}you can do
item.unwrap_or_else(|| something_else(n));
instead
(or
item.unwrap_or(10)
for constants or cheap evaluations)
There was a problem hiding this comment.
This pattern doesn't seem to work, since the None pattern here requires an async evaluation.
There was a problem hiding this comment.
mhmm that is true. we should just work on getting the page handlers set up so we can call that instead.
No description provided.