Refactor navigation helpers #465
Conversation
|
The changes look good at first glance. I am going to do a more in-depth review soon. |
| base_path: &String, | ||
| current_path: &String, | ||
| current_item: &StringMap, | ||
| previous_item: StringMap, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| t.render(r, &mut local_rc) | ||
| })?; | ||
| if let Some(previous) = previous { | ||
| if let Some(item) = target.find(&base_path, &path, &item, previous)? { |
There was a problem hiding this comment.
👍 This looks so much nicer than the massive block we had before!
| r: &Handlebars, | ||
| rc: &mut RenderContext, | ||
| chapter: &StringMap, | ||
| ) -> Result<(), RenderError> { |
There was a problem hiding this comment.
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?
This makes more sense for find as an interface, though it causes a second clone in some cases. Maybe rustc is smart here?
jacwah
force-pushed
the
refactor-navigation
branch
from
1ab0db8 to
6ca14cc
Compare
|
@Michael-F-Bryan I changed the signature of |
| h.register_helper("previous", Box::new(previous)); | ||
| h.register_helper("next", Box::new(next)); | ||
|
|
||
| assert_eq!( |
There was a problem hiding this comment.
I usually break this out into a couple lines. So at the top of the test I'll assign what I expect to a should_be variable, then assign what we actually get to got. Then at the bottom you can write assert_eq!(got, should_be); (e.g. config.rs).
It's definitely a personal taste thing, but it's nice when you can look at the top and figure out what the expected output should be at a glance without needing to look for all assert_eq!() calls.
The tests you've written look good to me. They're clear, concise, and test most common scenarios 👍 I restarted the Mac travis build. It looks like it was some spurious Travis issue that failed the build and not anything to do with you. Once travis gives the green light I reckon this is ready to merge. |
|
The failed build for OSX/beta is completely unrelated to this PR and seems to be some sort of permissions issue with our In the meantime I think it's fine to merge this PR. Nice work @jacwah! |
* Refactor navigation helpers * Target::find: take previous_item by reference This makes more sense for find as an interface, though it causes a second clone in some cases. Maybe rustc is smart here? * Test next and previous navigation helpers * Add more next/previous tests
This is my attempt at a piece of #458.
previous()andnext()have been removed.There are probably tons of potential improvements, but it still feels a lot cleaner than before 😊.