Skip to content
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.

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

Save formatted map to string #4. Also, apply cargo fmt #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
//! ```no_test
//! cargo add knossos
//! ```
//!
//!
//! Or add the following line to your `Cargo.toml`:
//! ```no_test
//! [dependencies]
Expand Down
35 changes: 20 additions & 15 deletions src/maze/formatters/game_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@ impl ExtraState for WithStartGoal {}

/// A GameMap formatter for a generated maze
///
/// This formatter is designed for generating game maps suitable for pseudo-3D games utilizing the ray-casting
/// algorithm for map modeling and rendering.
/// This formatter is designed for generating game maps suitable for pseudo-3D games utilizing the
/// ray-casting algorithm for map modeling and rendering.
///
/// By default, it generates a self-contained map without predefined start and exit points. However, it also offers
/// the option to randomly place the start and goal points along the map borders, ensuring a viable path between the
/// two points.
/// By default, it generates a self-contained map without predefined start and exit points. However,
/// it also offers the option to randomly place the start and goal points along the map borders,
/// ensuring a viable path between the two points.
///
/// # Examples:
///
Expand Down Expand Up @@ -82,7 +82,8 @@ impl GameMap<NoStartGoal> {
}
}

/// Returns a new instance of a [GameMap] formatter of a new type with an option to randonly spawn the start and goal characters on the borders of a map
/// Returns a new instance of a [GameMap] formatter of a new type with an option to randonly
/// spawn the start and goal characters on the borders of a map
pub fn with_start_goal(self) -> GameMap<WithStartGoal> {
GameMap {
state: self.state,
Expand Down Expand Up @@ -112,7 +113,8 @@ impl GameMap<NoStartGoal> {
}
}

/// An implementation of a formatter with the predefined start and goal points randomly placed along the map borders
/// An implementation of a formatter with the predefined start and goal points randomly placed along
/// the map borders
impl GameMap<WithStartGoal> {
/// Sets a goal charachter and returns itself
pub fn goal(mut self, goal: char) -> Self {
Expand Down Expand Up @@ -145,7 +147,8 @@ impl GameMap<WithStartGoal> {
.iter()
.filter(|(nrow, ncol)| *ncol != scol && *nrow != srow)
.nth(0)
.unwrap(); // the smallest grid with a single cell formatted into a map has 3 available positions for a goal
.unwrap(); // the smallest grid with a single cell formatted into a map has 3 available positions for a
// goal

let start_idx = srow * rows + scol;
let goal_idx = grow * rows + gcol;
Expand Down Expand Up @@ -427,17 +430,19 @@ mod tests {
#[test]
fn possible_start_and_goal_positions() {
let formatter = GameMap::new().with_start_goal();
#[cfg_attr(rustfmt, rustfmt_skip)]
let map = vec![
'#', '#', '#', '#', '#', '#', '#', '#', '#',
'#', '.', '.', '.', '#', '.', '.', '.', '#',
'#', '#', '#', '#', '#', '#', '#', '#', '#',
'#', '.', '.', '.', '#', '.', '.', '.', '#',
'#', '#', '#', '.', '#', '.', '#', '.', '#',
'#', '.', '.', '.', '#', '.', '#', '.', '#',
'#', '.', '#', '#', '#', '.', '#', '.', '#',
'#', '.', '.', '.', '.', '.', '#', '.', '#',
'#', '#', '#', '#', '#', '#', '#', '.', '#',
'#', '.', '.', '.', '.', '.', '.', '.', '#',
'#', '.', '.', '.', '#', '.', '#', '.', '#',
'#', '.', '#', '#', '#', '.', '#', '.', '#',
'#', '.', '.', '.', '.', '.', '#', '.', '#',
'#', '#', '#', '#', '#', '#', '#', '.', '#',
'#', '.', '.', '.', '.', '.', '.', '.', '#',
'#', '#', '#', '#', '#', '#', '#', '#', '#',
];
#[cfg_attr(rustfmt, rustfmt_skip)]
let positions = vec![
(0, 1), (0, 2), (0, 3), (0, 5), (0, 6),
(0, 7), (1, 0), (1, 8), (2, 8), (3, 0),
Expand Down
16 changes: 11 additions & 5 deletions src/maze/formatters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub trait Saveable {
/// In case of success, returns the string with a success message.
/// Otherwise, returns a [MazeSaveError] with a custom reason message.
fn save(&self, path: &str) -> Result<String, MazeSaveError>;
fn format(&self) -> String;
}

/// A custom wrapper over [RgbImage] for converting a maze to an image
Expand All @@ -46,6 +47,9 @@ impl Saveable for ImageWrapper {

Ok(format!("Maze was successfully saved as an image: {}", path))
}
fn format(&self) -> String {
std::string::String::new()
}
Comment on lines +50 to +52
Copy link
Owner

Choose a reason for hiding this comment

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

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:

image

This approach also eliminates the confusion caused by the Saveable trait being aware of the format method.

}

/// A custom wrapper over [std::string::String](std::string::String) for converting a maze into
Expand Down Expand Up @@ -75,15 +79,17 @@ impl Saveable for StringWrapper {
};

match file.write_all(self.0.as_bytes()) {
Err(why) => {
Err(MazeSaveError {
reason: format!("Couldn't write to {}: {}", path.display(), why),
})
}
Err(why) => Err(MazeSaveError {
reason: format!("Couldn't write to {}: {}", path.display(), why),
}),
Ok(_) => Ok(format!(
"Maze was successfully written to a file: {}",
path.display()
)),
}
}

fn format(&self) -> String {
self.0.clone()
}
}
6 changes: 1 addition & 5 deletions src/maze/grid/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,7 @@ impl Grid {
Ok((nx, ny))
}

pub fn get_next_cell_coords(
&self,
coords: Coords,
direction: Cell,
) -> TransitResult<Coords> {
pub fn get_next_cell_coords(&self, coords: Coords, direction: Cell) -> TransitResult<Coords> {
self.validate_transit(coords, direction)?;

let (x, y) = coords;
Expand Down
10 changes: 10 additions & 0 deletions src/maze/maze.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

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

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)
}
Comment on lines +46 to +53
Copy link
Owner

Choose a reason for hiding this comment

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

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.

}

impl fmt::Display for OrthogonalMaze {
Expand Down