Fix & add tests for template object loading #227
Conversation
Co-authored-by: Adam Aposhian <aposhian.dev@gmail.com>
It was being provided both file and directory paths while the code inside assumed it was a directory path only. By making the name clearer I think we'll avoid other issues like this one. |
Yes, but in my quick review I didn't notice the location where we were passing a file instead of a directory path. I'd personally prefer a patch that just fixes that location. |
| @@ -102,7 +102,7 @@ impl LayerData { | |||
| attrs, | |||
| Some(tilesets), | |||
| for_tileset, | |||
| map_path, | |||
| map_path.parent().ok_or(crate::Error::PathIsNotFile)?, | |||
There was a problem hiding this comment.
Could we pass in the parent directory to LayerData::new in the first place? Or does the map_path parameter to ImageLayerData::new and GroupLayerData::new need to be the actual map path?
There was a problem hiding this comment.
We could change that, but I was trying to change as few lines as possible since you asked to patch just the bug.
There was a problem hiding this comment.
Yeah, that's appreciated! It just highlighted that the inconsistency goes further than this one line. But, we can merge this fix and make it consistent in a follow-up change.
For following this up, I think we should always pass just the directory.
Fixes the underlying issue behind #224, and adds a few tests with a map provided by @Aposhian (Thank you!). It also makes
DefaultResourceCache's members public for easy access from tests and whatnot.