better documentation

src/game.rs

@@ -96,7 +96,8 @@ pub struct Game {
     pub room_format: Option<RoomFormat>,
     pub(crate) room_type: RoomType,
     pub font: Font,
-    pub custom_font: Option<String>, // used if font is Font::Custom
+    /// used if font is `Font::Custom`
+    pub custom_font: Option<String>,
     pub text_direction: TextDirection,
     pub palettes: Vec<Palette>,
     pub rooms: Vec<Room>,
@@ -107,7 +108,8 @@ pub struct Game {
     pub endings: Vec<Ending>,
     pub variables: Vec<Variable>,
     pub font_data: Option<String>, // todo make this an actual struct for parsing
-    pub(crate) line_endings_crlf: bool, // otherwise lf (unix/mac)
+    /// true if CRLF (Windows), otherwise LF (unix/mac)
+    pub(crate) line_endings_crlf: bool,
 }
 
 impl Game {
@@ -173,6 +175,7 @@ impl Game {
         let mut items: Vec<Item> = Vec::new();
         let mut avatar_exists = false;
 
+        // todo can we use multithreading here?
         for segment in segments {
             if segment.starts_with("# BITSY VERSION") {
                 let segment = segment.replace("# BITSY VERSION ", "");
@@ -340,13 +343,13 @@ impl Game {
         let mut tile_ids = room.tiles.clone();
         tile_ids.sort();
         tile_ids.dedup();
-        // remove 0 as this isn't a real tile
+
-        let zero_index  = tile_ids.iter()
+        // remove "0" as this isn't a real tile
-            .position(|i| i == "0");
+        if let Some(zero_index) = tile_ids.iter().position(|i| i == "0") {
-        if let Some(zero_index) = zero_index {
             tile_ids.remove(zero_index);
         }
-        // remove Ok once this function returns a result
+
+        // todo remove Ok once get_tiles_by_ids returns a Result
         Ok(self.get_tiles_by_ids(tile_ids))
     }
 
@@ -521,13 +524,13 @@ impl Game {
             self.add_room(room);
         }
 
-        // a sprite has a dialogue ID, so we need to handle these after dialogues
+        // a sprite has a dialogue ID, so we need to handle sprites after dialogues
-        // a sprite has a position in a room, so we need to handle these after the rooms
+        // a sprite has a position in a room, so we need to handle sprites after rooms
         for sprite in &game.sprites {
             let mut sprite = sprite.clone();
             // avoid having two avatars
             if sprite.id == "A" {
-                sprite.id = "0".to_string(); // just a default value for replacement
+                sprite.id = "0".to_string(); // just a default value for later replacement
             }
 
             if let Some(key) = sprite.dialogue_id.clone() {
@@ -576,7 +579,9 @@ impl ToString for Game {
         }
 
         for dialogue in &self.dialogues {
-            // this replacement is silly but see segments_from_string() for explanation
+            // some dialogues are multiline (starting/ending with `"""`) but have no contents
+            // and this kinda messes things up when trying to export unmodified dialogues
+            // in their original format for testing purposes
             segments.push(dialogue.to_string().replace("\"\"\"\n\"\"\"", ""));
         }
 
@@ -620,6 +625,7 @@ impl Game {
     pub fn sprite_ids(&self) -> Vec<String> {
         self.sprites.iter().map(|sprite| sprite.id.clone()).collect()
     }
+
     pub fn room_ids(&self) -> Vec<String> {
         self.rooms.iter().map(|room| room.id.clone()).collect()
     }
@@ -866,7 +872,10 @@ mod test {
 
     #[test]
     fn game_from_string() {
-        let (output, _) = Game::from(include_str!["test-resources/default.bitsy"].to_string()).unwrap();
+        let (output, _error) = Game::from(
+            include_str!["test-resources/default.bitsy"].to_string()
+        ).unwrap();
+
         let expected = crate::mock::game_default();
 
         assert_eq!(output, expected);

src/tile.rs

@@ -5,14 +5,19 @@ use crate::image::animation_frames_from_str;
 pub struct Tile {
     pub id: String,
     pub name: Option<String>,
-    /// this is "optional" in that a tile can have `WAL true`, `WAL false` or neither
+    /// Can the player move over this tile?
-    /// obviously Some(false) is the same as None but we want to preserve the original formatting
+    /// This is "optional" in that a tile can have `WAL true`, `WAL false` or neither.
+    /// obviously Some(false) is functionally the same as None
+    /// but we want to preserve the original formatting where possible.
     pub wall: Option<bool>,
     pub animation_frames: Vec<Image>,
+    /// Bitsy has an undocumented feature where a tile can be rendered
+    /// in a specific colour (`COL n`) from the current palette.
     pub colour_id: Option<u64>,
 }
 
 impl PartialEq for Tile {
+    /// ignore id and name.
     fn eq(&self, other: &Self) -> bool {
         self.wall == other.wall
         &&
@@ -44,6 +49,7 @@ impl Tile {
     }
 
     // todo refactor
+    // can we do map_in_place or something?
 
     pub fn invert(&mut self) {
         self.animation_frames = self.animation_frames.iter().map(|frame: &Image| {
@@ -162,8 +168,7 @@ mod test {
                 mock::image::chequers_2(),
             ],
             colour_id: None,
-        }
+        }.to_string();
-            .to_string();
 
         let expected = include_str!("test-resources/tile-chequers").to_string();