Skip to content

Refactor navigation helpers #465

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 18, 2017
Merged

Refactor navigation helpers #465

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0435ac
Refactor navigation helpers
jacwah Oct 6, 2017
31982bc
Target::find: take previous_item by reference
jacwah Nov 13, 2017
7ffb4ab
Test next and previous navigation helpers
jacwah Nov 13, 2017
6ca14cc
Add more next/previous tests
jacwah Nov 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
230 changes: 99 additions & 131 deletions src/renderer/html_handlebars/helpers/navigation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,165 +5,133 @@ use serde_json;
use handlebars::{Context, Handlebars, Helper, RenderContext, RenderError, Renderable};


// Handlebars helper for navigation
type StringMap = BTreeMap<String, String>;

pub fn previous(_h: &Helper, r: &Handlebars, rc: &mut RenderContext) -> Result<(), RenderError> {
debug!("[fn]: previous (handlebars helper)");
/// Target for `find_chapter`.
enum Target {
Previous,
Next,
}

impl Target {
/// Returns target if found.
fn find(&self,
base_path: &String,
current_path: &String,
current_item: &StringMap,
previous_item: StringMap,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why we can't take previous_item by reference here? It feels a bit odd that we're taking the current item by reference but then moving the previous item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was probably influenced by the fact that the call site already has previous_item available. This way we avoid a second clone. When looking at Target::find in isolation, that doesn't make a lot of though, as you point out.

) -> Result<Option<StringMap>, RenderError> {
match self {
&Target::Next => {
let previous_path = previous_item.get("path").ok_or_else(|| {
RenderError::new("No path found for chapter in JSON data")
})?;

if previous_path == base_path {
return Ok(Some(current_item.clone()));
}
},

&Target::Previous => {
if current_path == base_path {
return Ok(Some(previous_item));
}
}
}

Ok(None)
}
}

fn find_chapter(
rc: &mut RenderContext,
target: Target
) -> Result<Option<StringMap>, RenderError> {
debug!("[*]: Get data from context");

let chapters = rc.evaluate_absolute("chapters").and_then(|c| {
serde_json::value::from_value::<Vec<BTreeMap<String, String>>>(c.clone())
serde_json::value::from_value::<Vec<StringMap>>(c.clone())
.map_err(|_| RenderError::new("Could not decode the JSON data"))
})?;

let current = rc.evaluate_absolute("path")?
.as_str()
.ok_or_else(|| RenderError::new("Type error for `path`, string expected"))?
.replace("\"", "");
let base_path = rc.evaluate_absolute("path")?
.as_str()
.ok_or_else(|| RenderError::new("Type error for `path`, string expected"))?
.replace("\"", "");

let mut previous: Option<StringMap> = None;

let mut previous: Option<BTreeMap<String, String>> = None;
debug!("[*]: Search for chapter");

debug!("[*]: Search for current Chapter");
// Search for current chapter and return previous entry
for item in chapters {
match item.get("path") {
Some(path) if !path.is_empty() => {
if path == &current {
debug!("[*]: Found current chapter");
if let Some(previous) = previous {
debug!("[*]: Creating BTreeMap to inject in context");
// Create new BTreeMap to extend the context: 'title' and 'link'
let mut previous_chapter = BTreeMap::new();

// Chapter title
previous.get("name")
.ok_or_else(|| {
RenderError::new("No title found for chapter in \
JSON data")
})
.and_then(|n| {
previous_chapter.insert("title".to_owned(), json!(n));
Ok(())
})?;


// Chapter link
previous.get("path")
.ok_or_else(|| {
RenderError::new("No path found for chapter in \
JSON data")
})
.and_then(|p| {
Path::new(p).with_extension("html")
.to_str()
.ok_or_else(|| {
RenderError::new("Link could not be \
converted to str")
})
.and_then(|p| {
previous_chapter
.insert("link".to_owned(), json!(p.replace("\\", "/")));
Ok(())
})
})?;


debug!("[*]: Render template");
// Render template
_h.template()
.ok_or_else(|| RenderError::new("Error with the handlebars template"))
.and_then(|t| {
let mut local_rc = rc.with_context(Context::wraps(&previous_chapter)?);
t.render(r, &mut local_rc)
})?;
if let Some(previous) = previous {
if let Some(item) = target.find(&base_path, &path, &item, previous)? {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This looks so much nicer than the massive block we had before!

return Ok(Some(item));
}
break;
} else {
previous = Some(item.clone());
}

previous = Some(item.clone());
}
_ => continue,
}
}

Ok(None)
}

fn render(
_h: &Helper,
r: &Handlebars,
rc: &mut RenderContext,
chapter: &StringMap,
) -> Result<(), RenderError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

How hard would it be to write a test which checks this handlebars helper? Would it be possible to mock up a quick template then run the handlebars helper and assert that the rendered output looks like we expect it to?

debug!("[*]: Creating BTreeMap to inject in context");

let mut context = BTreeMap::new();

chapter.get("name")
.ok_or_else(|| RenderError::new("No title found for chapter in JSON data"))
.map(|name| context.insert("title".to_owned(), json!(name)))?;

chapter.get("path")
.ok_or_else(|| RenderError::new("No path found for chapter in JSON data"))
.and_then(|p| {
Path::new(p).with_extension("html")
.to_str()
.ok_or_else(|| RenderError::new("Link could not be converted to str"))
.map(|p| context.insert("link".to_owned(), json!(p.replace("\\", "/"))))
})?;

debug!("[*]: Render template");

_h.template()
.ok_or_else(|| RenderError::new("Error with the handlebars template"))
.and_then(|t| {
let mut local_rc = rc.with_context(Context::wraps(&context)?);
t.render(r, &mut local_rc)
})?;

Ok(())
}

pub fn previous(_h: &Helper, r: &Handlebars, rc: &mut RenderContext) -> Result<(), RenderError> {
debug!("[fn]: previous (handlebars helper)");

if let Some(previous) = find_chapter(rc, Target::Previous)? {
render(_h, r, rc, &previous)?;
}

Ok(())
}

pub fn next(_h: &Helper, r: &Handlebars, rc: &mut RenderContext) -> Result<(), RenderError> {
debug!("[fn]: next (handlebars helper)");

debug!("[*]: Get data from context");
let chapters = rc.evaluate_absolute("chapters").and_then(|c| {
serde_json::value::from_value::<Vec<BTreeMap<String, String>>>(c.clone())
.map_err(|_| RenderError::new("Could not decode the JSON data"))
})?;
let current = rc.evaluate_absolute("path")?
.as_str()
.ok_or_else(|| RenderError::new("Type error for `path`, string expected"))?
.replace("\"", "");

let mut previous: Option<BTreeMap<String, String>> = None;

debug!("[*]: Search for current Chapter");
// Search for current chapter and return previous entry
for item in chapters {
match item.get("path") {
Some(path) if !path.is_empty() => {
if let Some(previous) = previous {
let previous_path = previous.get("path").ok_or_else(|| {
RenderError::new("No path found for chapter in JSON data")
})?;

if previous_path == &current {
debug!("[*]: Found current chapter");
debug!("[*]: Creating BTreeMap to inject in context");
// Create new BTreeMap to extend the context: 'title' and 'link'
let mut next_chapter = BTreeMap::new();

item.get("name")
.ok_or_else(|| {
RenderError::new("No title found for chapter in JSON \
data")
})
.and_then(|n| {
next_chapter.insert("title".to_owned(), json!(n));
Ok(())
})?;

Path::new(path).with_extension("html")
.to_str()
.ok_or_else(|| {
RenderError::new("Link could not converted \
to str")
})
.and_then(|l| {
debug!("[*]: Inserting link: {:?}", l);
// Hack for windows who tends to use `\` as separator instead of `/`
next_chapter.insert("link".to_owned(), json!(l.replace("\\", "/")));
Ok(())
})?;

debug!("[*]: Render template");

// Render template
_h.template()
.ok_or_else(|| RenderError::new("Error with the handlebars template"))
.and_then(|t| {
let mut local_rc = rc.with_context(Context::wraps(&next_chapter)?);
t.render(r, &mut local_rc)
})?;
break;
}
}

previous = Some(item.clone());
}

_ => continue,
}
if let Some(next) = find_chapter(rc, Target::Next)? {
render(_h, r, rc, &next)?;
}

Ok(())
}