make a start toward better error handling

src/colour.rs

@@ -5,15 +5,12 @@ pub struct Colour {
     pub blue: u8,
 }
 
-#[derive(Debug)]
-pub struct InvalidRgb;
-
 impl Colour {
-    pub(crate) fn from(string: &str) -> Result<Colour, InvalidRgb> {
+    pub(crate) fn from(string: &str) -> Result<Colour, crate::Error> {
         let values: Vec<&str> = string.trim_matches(',').split(',').collect();
 
         if values.len() != 3 {
-            return Err(InvalidRgb);
+            return Err(crate::Error::Colour);
         }
 
         let red: u8 = values[0].parse().unwrap_or(0);

src/error.rs

@@ -0,0 +1,51 @@
+use std::fmt;
+
+#[derive(Debug, PartialEq)]
+pub enum NotFound {
+    Anything,
+    Avatar,
+    Room,
+    Sprite,
+    Tile,
+}
+
+impl fmt::Display for NotFound {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f,"Not found: {} data", match self {
+            NotFound::Anything => "game",
+            NotFound::Avatar   => "avatar",
+            NotFound::Room     => "room",
+            NotFound::Sprite   => "sprite",
+            NotFound::Tile     => "tile",
+        })
+    }
+}
+
+#[derive(Debug)]
+pub enum Error {
+    Colour,
+    Dialogue,
+    Ending,
+    Exit,
+    Game {
+        missing: NotFound,
+    },
+    Image,
+    Item,
+    Palette,
+    Position,
+    Room,
+    Sprite,
+    Text,
+    Tile,
+    Variable,
+    Version,
+}
+
+impl fmt::Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "")
+    }
+}
+
+impl std::error::Error for Error {}

src/game.rs

@@ -1,4 +1,4 @@
-use crate::{Dialogue, Ending, Font, Image, Item, Palette, Room, Sprite, TextDirection, Tile, Variable, transform_line_endings, segments_from_string, new_unique_id, try_id, Instance};
+use crate::{Dialogue, Ending, Font, Image, Item, Palette, Room, Sprite, TextDirection, Tile, Variable, transform_line_endings, segments_from_string, new_unique_id, try_id, Instance, Error};
 
 use loe::TransformMode;
 
@@ -6,7 +6,6 @@ use std::str::FromStr;
 use std::collections::HashMap;
 use std::borrow::BorrowMut;
 use std::fmt;
-use std::fmt::Display;
 
 /// in very early versions of Bitsy, room tiles were defined as single alphanumeric characters -
 /// so there was a maximum of 36 unique tiles. later versions are comma-separated.
@@ -27,11 +26,11 @@ impl RoomFormat {
     }
 }
 
-impl Display for RoomFormat {
+impl fmt::Display for RoomFormat {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(f, "{}", match &self {
-            RoomFormat::Contiguous     => "0",
+            RoomFormat::Contiguous     => 0,
-            RoomFormat::CommaSeparated => "1",
+            RoomFormat::CommaSeparated => 1,
         })
     }
 }
@@ -56,42 +55,37 @@ pub struct Version {
 }
 
 #[derive(Debug)]
-pub struct InvalidVersion;
+pub enum VersionError {
+    MissingParts,
+    ExtraneousParts,
+    MalformedInteger,
+}
 
-impl Version {
+impl fmt::Display for VersionError {
-    fn from(str: &str) -> Result<Version, InvalidVersion> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let parts: Vec<&str> = str.split('.').collect();
+        write!(f, "{}", match self {
-
+            VersionError::MissingParts     => "Not enough parts supplied for version",
-        if parts.len() == 2 {
+            VersionError::ExtraneousParts  => "Too many parts supplied for version",
-            Ok(Version {
+            VersionError::MalformedInteger => "Version did not contain valid integers",
-                major: parts[0].parse().unwrap(),
+        })
-                minor: parts[1].parse().unwrap(),
-            })
-        } else {
-            Err (InvalidVersion)
-        }
     }
 }
 
-#[derive(Debug, PartialEq)]
+impl std::error::Error for VersionError {}
-pub enum NotFound {
-    /// no game data whatsoever
-    Anything,
-    Avatar,
-    Room,
-    Sprite,
-    Tile,
-}
 
-impl Display for NotFound {
+impl Version {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    fn from(str: &str) -> Result<Version, VersionError> {
-        write!(f,"Not found: {} data", match self {
+        let parts: Vec<&str> = str.split('.').collect();
-            NotFound::Anything => "game",
+
-            NotFound::Avatar   => "avatar",
+        if parts.len() < 2 {
-            NotFound::Room     => "room",
+            Err(VersionError::MissingParts)
-            NotFound::Sprite   => "sprite",
+        } else if parts.len() > 2 {
-            NotFound::Tile     => "tile",
+            Err(VersionError::ExtraneousParts)
-        })
+        } else if let (Ok(major), Ok(minor)) = (parts[0].parse(), parts[1].parse()) {
+            Ok(Version { major, minor })
+        } else {
+            Err(VersionError::MalformedInteger)
+        }
     }
 }
 
@@ -116,19 +110,14 @@ pub struct Game {
     pub(crate) line_endings_crlf: bool, // otherwise lf (unix/mac)
 }
 
-#[derive(Debug)]
-pub struct GameHasNoAvatar;
-// todo no tiles? no rooms? no palettes? turn this into an enum?
-
 impl Game {
-    // todo return (Result<Game, ?>, Vec<Box<dyn Error>>)?
+    pub fn from(string: String) -> Result<(Game, Vec<crate::Error>), crate::error::NotFound> {
-    // would be nice to *try* to parse a game, and catalogue any and all errors without crashing,
-    // for display purposes etc.
-    pub fn from(string: String) -> Result<Game, NotFound> {
         if string.trim() == "" {
-            return Err(NotFound::Anything);
+            return Err(crate::error::NotFound::Anything);
         }
 
+        let mut warnings = Vec::new();
+
         let line_endings_crlf = string.contains("\r\n");
         let mut string = string;
         if line_endings_crlf {
@@ -182,14 +171,17 @@ impl Game {
         let mut tiles: Vec<Tile> = Vec::new();
         let mut sprites: Vec<Sprite> = Vec::new();
         let mut items: Vec<Item> = Vec::new();
-        let mut avatar_exists = false;
+        // let mut avatar_exists = false;
 
         for segment in segments {
             if segment.starts_with("# BITSY VERSION") {
                 let segment = segment.replace("# BITSY VERSION ", "");
-                let segment = Version::from(&segment);
+                let result = Version::from(&segment);
-                if let Ok(segment) = segment {
+
-                    version = Some(segment);
+                if let Ok(v) = result {
+                    version = Some(v);
+                } else {
+                    warnings.push(Error::Version);
                 }
             } else if segment.starts_with("! ROOM_FORMAT") {
                 let segment = segment.replace("! ROOM_FORMAT ", "");
@@ -207,7 +199,13 @@ impl Game {
             } else if segment.trim() == "TEXT_DIRECTION RTL" {
                 text_direction = TextDirection::RightToLeft;
             } else if segment.starts_with("PAL ") {
-                palettes.push(Palette::from(segment));
+                let result = Palette::from_str(&segment);
+                if let Ok((palette, mut errors)) = result {
+                    palettes.push(palette);
+                    warnings.append(&mut errors);
+                } else {
+                    warnings.push(result.unwrap_err());
+                }
             } else if segment.starts_with("ROOM ") || segment.starts_with("SET ") {
                 if segment.starts_with("SET ") {
                     room_type = RoomType::Set;
@@ -219,7 +217,7 @@ impl Game {
                 let sprite = Sprite::from(segment);
 
                 if let Ok(sprite) = sprite {
-                    avatar_exists |= sprite.id == "A";
+                    // avatar_exists |= sprite.id == "A";
 
                     sprites.push(sprite);
                 }
@@ -238,68 +236,71 @@ impl Game {
             }
         }
 
-        if ! avatar_exists {
+        // if ! avatar_exists {
-            return Err(NotFound::Avatar);
+        //     return Err(crate::Error::NotFound::Avatar);
-        }
+        // }
 
         Ok(
-            Game {
+            (
-                name,
+                Game {
-                version,
+                    name,
-                room_format,
+                    version,
-                room_type,
+                    room_format,
-                font,
+                    room_type,
-                custom_font,
+                    font,
-                text_direction,
+                    custom_font,
-                palettes,
+                    text_direction,
-                rooms,
+                    palettes,
-                tiles,
+                    rooms,
-                sprites,
+                    tiles,
-                items,
+                    sprites,
-                dialogues,
+                    items,
-                endings,
+                    dialogues,
-                variables,
+                    endings,
-                font_data,
+                    variables,
-                line_endings_crlf,
+                    font_data,
-            }
+                    line_endings_crlf,
+                },
+                warnings
+            )
         )
     }
 
     /// todo refactor this into "get T by ID", taking a Vec<T> and an ID name?
-    pub fn get_sprite_by_id(&self, id: String) -> Result<&Sprite, NotFound> {
+    pub fn get_sprite_by_id(&self, id: String) -> Result<&Sprite, crate::error::NotFound> {
         let index = self.sprites.iter().position(
             |sprite| sprite.id == id
         );
 
         match index {
             Some(index) => Ok(&self.sprites[index]),
-            None => Err(NotFound::Sprite),
+            None => Err(crate::error::NotFound::Sprite),
         }
     }
 
-    pub fn get_tile_by_id(&self, id: String) -> Result<&Tile, NotFound> {
+    pub fn get_tile_by_id(&self, id: String) -> Result<&Tile, crate::error::NotFound> {
         let index = self.tiles.iter().position(
             |tile| tile.id == id
         );
 
         match index {
             Some(index) => Ok(&self.tiles[index]),
-            None => Err(NotFound::Tile),
+            None => Err(crate::error::NotFound::Tile),
         }
     }
 
-    pub fn get_room_by_id(&self, id: String) -> Result<&Room, NotFound> {
+    pub fn get_room_by_id(&self, id: String) -> Result<&Room, crate::error::NotFound> {
         let index = self.rooms.iter().position(
             |room| room.id == id
         );
 
         match index {
             Some(index) => Ok(&self.rooms[index]),
-            None => Err(NotFound::Room),
+            None => Err(crate::error::NotFound::Room),
         }
     }
 
-    pub fn get_avatar(&self) -> Result<&Sprite, NotFound> {
+    pub fn get_avatar(&self) -> Result<&Sprite, crate::error::NotFound> {
         self.get_sprite_by_id("A".to_string())
     }
 
@@ -316,12 +317,9 @@ impl Game {
         tiles
     }
 
-    pub fn get_tiles_for_room(&self, id: String) -> Result<Vec<&Tile>, NotFound> {
+    pub fn get_tiles_for_room(&self, id: String) -> Result<Vec<&Tile>, crate::error::NotFound> {
-        let room = self.get_room_by_id(id);
+        let room = self.get_room_by_id(id)?;
-        if room.is_err() {
+        let mut tile_ids = room.tiles.clone();
-            return Err(NotFound::Room);
-        }
-        let mut tile_ids = room.unwrap().tiles.clone();
         tile_ids.sort();
         tile_ids.dedup();
         // remove 0 as this isn't a real tile
@@ -842,11 +840,11 @@ impl Game {
 
 #[cfg(test)]
 mod test {
-    use crate::{TextDirection, Font, Version, Game, NotFound, Tile, Image};
+    use crate::{TextDirection, Font, Version, Game, Tile, Image};
 
     #[test]
     fn game_from_string() {
-        let output = Game::from(include_str!["test-resources/default.bitsy"].to_string()).unwrap();
+        let (output, _) = Game::from(include_str!["test-resources/default.bitsy"].to_string()).unwrap();
         let expected = crate::mock::game_default();
 
         assert_eq!(output, expected);
@@ -909,7 +907,7 @@ mod test {
 
     #[test]
     fn arabic() {
-        let game = Game::from(include_str!("test-resources/arabic.bitsy").to_string()).unwrap();
+        let (game, _) = Game::from(include_str!("test-resources/arabic.bitsy").to_string()).unwrap();
 
         assert_eq!(game.font, Font::Arabic);
         assert_eq!(game.text_direction, TextDirection::RightToLeft);
@@ -1067,7 +1065,7 @@ mod test {
 
     #[test]
     fn empty_game_data_throws_error() {
-        assert_eq!(Game::from("".to_string()        ).err().unwrap(), NotFound::Anything);
+        assert_eq!(Game::from("".to_string()        ).unwrap_err(), crate::error::NotFound::Anything);
-        assert_eq!(Game::from(" \n \r\n".to_string()).err().unwrap(), NotFound::Anything);
+        assert_eq!(Game::from(" \n \r\n".to_string()).unwrap_err(), crate::error::NotFound::Anything);
     }
 }

src/lib.rs

@@ -7,6 +7,7 @@ use loe::{process, Config, TransformMode};
 pub mod colour;
 pub mod dialogue;
 pub mod ending;
+pub mod error;
 pub mod exit;
 pub mod game;
 pub mod image;
@@ -24,6 +25,7 @@ pub mod test_omnibus;
 pub use colour::Colour;
 pub use dialogue::Dialogue;
 pub use ending::Ending;
+pub use error::Error;
 pub use exit::*;
 pub use game::*;
 pub use image::Image;

src/palette.rs

@@ -1,4 +1,4 @@
-use crate::colour::Colour;
+use crate::Colour;
 
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Palette {
@@ -7,25 +7,39 @@ pub struct Palette {
     pub colours: Vec<Colour>,
 }
 
-impl From<String> for Palette {
+impl Palette {
-    fn from(string: String) -> Palette {
+    pub fn from_str(s: &str) -> Result<(Palette, Vec<crate::Error>), crate::Error> {
-        let lines: Vec<&str> = string.lines().collect();
+        let mut lines: Vec<&str> = s.lines().collect();
 
-        let id = lines[0].replace("PAL ", "");
+        if lines.is_empty() {
+            return Err(crate::Error::Palette);
+        }
 
-        let name = match lines[1].starts_with("NAME") {
+        let mut id = String::new();
-            true  => Some(lines[1].replace("NAME ", "")),
+        let mut name = None;
-            false => None,
+        let mut colours = Vec::new();
-        };
+        let mut warnings = Vec::new();
 
-        let colour_start_index = if name.is_some() { 2 } else { 1 };
+        while !lines.is_empty() {
+            let line = lines.pop().unwrap();
 
-        let colours = lines[colour_start_index..]
+            if line.starts_with("PAL ") {
-            .iter()
+                id = line.replace("PAL ", "");
-            .map(|&line| Colour::from(line).unwrap())
+            } else if line.starts_with("NAME ") {
-            .collect();
+                name = Some(line.replace("NAME ", ""));
+            } else {
+                let result = Colour::from(line);
+                if let Ok(colour) = result {
+                    colours.push(colour)
+                } else {
+                    warnings.push(result.unwrap_err());
+                }
+            }
+        }
 
-        Palette { id, name, colours }
+        colours.reverse();
+
+        Ok((Palette { id, name, colours }, warnings))
     }
 }
 
@@ -53,7 +67,7 @@ mod test {
 
     #[test]
     fn palette_from_string() {
-        let output = Palette::from("PAL 1\nNAME lamplight\n45,45,59\n66,60,39\n140,94,1".to_string());
+        let (output, _) = Palette::from_str("PAL 1\nNAME lamplight\n45,45,59\n66,60,39\n140,94,1").unwrap();
 
         let expected = Palette {
             id: "1".to_string(),
@@ -82,7 +96,7 @@ mod test {
 
     #[test]
     fn palette_from_string_no_name() {
-        let output = Palette::from("PAL 9\n45,45,59\n66,60,39\n140,94,1".to_string());
+        let (output, _) = Palette::from_str("PAL 9\n45,45,59\n66,60,39\n140,94,1").unwrap();
 
         let expected = Palette {
             id: "9".to_string(),
@@ -131,9 +145,9 @@ mod test {
                     blue: 128,
                 },
             ],
-        }
+        }.to_string();
-            .to_string();
+
-        let expected = "PAL g\nNAME moss\n1,2,3\n255,254,253\n126,127,128".to_string();
+        let expected = "PAL g\nNAME moss\n1,2,3\n255,254,253\n126,127,128";
         assert_eq!(output, expected);
     }
 }

src/test_omnibus.rs

@@ -55,7 +55,7 @@ mod test {
 
         assert!(result.is_ok());
 
-        let game = result.expect("failed to parse game");
+        let (game, _) = result.expect("failed to parse game");
 
         if ACCEPTED_FAILURES.contains(&id) {
             return;