Save formatted map to string #4. Also, apply cargo fmt
#5
Conversation
There was a problem hiding this comment.
Hi @jpedrick. Thanks for this PR. I'd be happy to merge your changes once you deal with my review comments. I'm open to discuss any follow-up questions of yours.
May I also ask you to write the commit(s) messages to match the conventional commits spec? Check the repo history for more examples.
| fn format(&self) -> String { | ||
| std::string::String::new() | ||
| } |
There was a problem hiding this comment.
The format method in ImageWrapper seems like a workaround. In the context of images, the string formatter doesn't make much sense, does it? And documenting this behavior complicates the library API.
Consider letting Rust handle this more idiomatically at compile time. To achieve this, we only need StringWrapper to implement another trait (e.g. custom Display or MazeDisplay), like this:
This approach also eliminates the confusion caused by the Saveable trait being aware of the format method.
| @@ -41,6 +41,16 @@ impl OrthogonalMaze { | |||
| let data = formatter.format(&self.grid); | |||
| Saveable::save(&data, path) | |||
| } | |||
|
|
|||
| // Saves a maze into a file to a given path using a given formatter | |||
There was a problem hiding this comment.
This should be changed to match the format method implementation.
| pub fn format<F, T>(&self, formatter: F) -> String | ||
| where | ||
| F: Formatter<T>, | ||
| T: Saveable, | ||
| { | ||
| let data = formatter.format(&self.grid); | ||
| Saveable::format(&data) | ||
| } |
There was a problem hiding this comment.
I'm concerned that this PR could potentially decrease the overall code coverage percentage, leading to CI check failures. I'd appreciate it if you add some integration tests to tests/maze.rs to ensure comprehensive coverage for the newly introduced logic.
Enable saving a map to a string using formatters to bypass file output.