Bad sprite dir warnings and svg validation #2211

nuts-rice · 2025-09-19T17:55:45Z

Addresses #2136 by:

Adding errors for empty dirs, invalid svg format + Failed Dir validation
Doing basic SVG format validation and warning on fails for individual files
Then validates whole directory , warns if empty, and provides summary of validated or failed files, tracks files that appear to be pre-generated
comprehensive warning messages if files appear to be pre-generated

Please let me know if this ought to be edited or the validation could be moved elsewhere.
Thanks!

for more information, see https://pre-commit.ci

Copilot

Pull Request Overview

This PR implements comprehensive sprite directory validation by adding SVG format checking and detailed warning messages for empty directories and pre-generated sprite files. It addresses issue #2136 by providing better feedback when sprite sources are misconfigured.

Adds validation for sprite source directories including SVG format checking
Introduces detailed error handling for empty directories and invalid SVG files
Provides comprehensive warnings when directories contain pre-generated sprite files instead of source SVGs

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
martin-core/src/resources/sprites/mod.rs	Implements directory validation, SVG format checking, and enhanced warning messages for sprite sources
martin-core/src/resources/sprites/error.rs	Adds new error types for empty directories, invalid SVG format, and directory validation failures

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T17:57:00Z

martin-core/src/resources/sprites/mod.rs

+                                    let filename = entry_path
+                                        .file_stem()
+                                        .map(|s| s.to_string_lossy().to_string())
+                                        .unwrap_or_default();


This logic for detecting pre-generated sprite files is duplicated in multiple places (lines 103-110, 190-197). Consider extracting this into a helper function to improve maintainability.

Copilot · 2025-09-19T17:57:00Z

martin-core/src/resources/sprites/mod.rs

+
+    /// Validates an individual SVG file for format.
+    async fn validate_svg_file(&self, path: &PathBuf) -> Result<(), SpriteError> {
+        let on_err = |e| SpriteError::IoError(e, path.clone());


The SVG validation is too restrictive. Valid SVG files can start with whitespace, comments, or DOCTYPE declarations before the <?xml or <svg tags. Consider using trim() before checking or using a more flexible approach.

let content = content.trim();
is already before the check, correct?

Copilot · 2025-09-19T17:57:00Z

martin-core/src/resources/sprites/mod.rs

+    }
+
+    /// Validates a sprite source directory to ensure it contains valid SVG files.
+    /// Checks include:


The parameter should be path: &Path instead of path: &PathBuf. Taking a reference to PathBuf is unnecessary since Path is the borrowed version that should be used in function parameters.

Copilot · 2025-09-19T17:57:01Z

martin-core/src/resources/sprites/mod.rs

+            } else {
+                warn!(
+                    "No SVG files found in sprite directory: {disp_path}. Please ensure the directory contains .svg files."
+                );


The parameter should be path: &Path instead of path: &PathBuf. Taking a reference to PathBuf is unnecessary since Path is the borrowed version that should be used in function parameters.

Copilot · 2025-09-19T17:57:01Z

martin-core/src/resources/sprites/mod.rs

+        } else {
+            warn!(
+                "Sprite source validation completed with errors: {}/{} sources valid, {} errors encountered",
+                valid_sources,


The parameter should be path: &Path instead of path: &PathBuf. Taking a reference to PathBuf is unnecessary since Path is the borrowed version that should be used in function parameters.

…rtin into sprite_dir_warnings

for more information, see https://pre-commit.ci

CommanderStorm

Why is there so much duplicate code? This does not seem very maintainable.
Please do a polishing pass 😉

Let's also likely change add_sprite to be a Result<(), SpritError> function as users likely want this instead.

CommanderStorm · 2025-09-20T09:24:40Z

martin-core/src/resources/sprites/mod.rs

+            return;
+        }
+
+        match std::fs::read_dir(&path) {


Let's move this validation to a new function. It is kind of messy to read otherwise

nyurik · 2025-09-21T16:12:33Z

martin-core/src/resources/sprites/mod.rs

-                        v.key(),
-                        v.get().path.display()
-                    );
+            return Err(SpriteError::DirectoryValidationFailed(


a warning is mutually exclusive with an error - either it fails, in which case the error message will be printed or handled by the caller, or it continues the execution but prints a warning message. Both do not make sense

CommanderStorm · 2025-09-21T17:01:14Z

martin-core/src/resources/sprites/mod.rs

+    }
+
+    /// Validates all configured sprite sources and logs a summary.
+    pub async fn validate_all_sources(&self) -> Result<(), SpriteError> {


validate_all_sources is never used.
As I asked before, please do a polishing pass.

nyurik · 2025-09-22T17:06:41Z

martin-core/src/resources/sprites/mod.rs

-                    );
+            return Err(SpriteError::DirectoryValidationFailed(
+                path,
+                "Path is not a directory".to_string(),


a minor nit, but makes for a better pattern -- if you have several similar errors with some magical string, easier to just have dedicated errors one for each. This way they are much faster to create (you don't need to allocate a new string on each error), and all content of an error is in the error description only

CommanderStorm

Sorry, but this still needs a deeper scrubbing.

Please add a unit test for each new behaviour (to prevent regressions and go over the code you are trying to add again.

CommanderStorm · 2025-09-22T19:07:36Z

martin-core/src/resources/sprites/mod.rs

+    /// - Directory existence and accessibility
+    /// - Presence of SVG files
+    /// - Basic SVG format validation
+    pub async fn validate_source_directory(&self, path: &PathBuf) -> Result<(), SpriteError> {


Let's not add dead code.
As I said above, this is never used in the codebase.

CommanderStorm · 2025-09-22T19:08:54Z

martin-core/src/resources/sprites/mod.rs

    async fn test_sprites() {
        let mut sprites = SpriteSources::default();
-        sprites.add_source(
+        let _ = sprites.add_source(


Let's unwrap instead to make sure this is not an error

for more information, see https://pre-commit.ci

feat: add sprite dir errors + warns + validation

08c3c0c

Copilot AI review requested due to automatic review settings September 19, 2025 17:55

[pre-commit.ci] auto fixes from pre-commit.com hooks

7cff982

for more information, see https://pre-commit.ci

Copilot AI reviewed Sep 19, 2025

View reviewed changes

nuts-rice and others added 3 commits September 19, 2025 12:14

chore: clippy + fmt

8f339a1

Merge branch 'sprite_dir_warnings' of https://github.com/nuts-rice/ma…

0b46368

…rtin into sprite_dir_warnings

[pre-commit.ci] auto fixes from pre-commit.com hooks

ef0fe33

for more information, see https://pre-commit.ci

CommanderStorm requested changes Sep 20, 2025

View reviewed changes

feat: move sprite validation + unclutter

3db4484

nuts-rice force-pushed the sprite_dir_warnings branch from ef0fe33 to 3db4484 Compare September 21, 2025 15:45

nyurik reviewed Sep 21, 2025

View reviewed changes

CommanderStorm reviewed Sep 21, 2025

View reviewed changes

feat: remove unused sprite validation + warning

1631a89

nuts-rice force-pushed the sprite_dir_warnings branch from 3f1c5f7 to 1631a89 Compare September 22, 2025 15:26

chore: clippy

5519fb3

nyurik reviewed Sep 22, 2025

View reviewed changes

feat: add sprite validation dedicated errors

abb1e01

nuts-rice force-pushed the sprite_dir_warnings branch from a4ded48 to abb1e01 Compare September 22, 2025 17:33

CommanderStorm reviewed Sep 22, 2025

View reviewed changes

CommanderStorm marked this pull request as draft September 23, 2025 00:44

tests: add sprite validation tests

af5b91d

nuts-rice force-pushed the sprite_dir_warnings branch from 2cd72c6 to af5b91d Compare September 24, 2025 16:25

[pre-commit.ci] auto fixes from pre-commit.com hooks

53cb0d8

for more information, see https://pre-commit.ci

Uh oh!

Bad sprite dir warnings and svg validation #2211

Are you sure you want to change the base?

Bad sprite dir warnings and svg validation #2211

Uh oh!

Conversation

nuts-rice commented Sep 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

nuts-rice Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

CommanderStorm left a comment

Choose a reason for hiding this comment

Uh oh!

CommanderStorm Sep 20, 2025

Choose a reason for hiding this comment

Uh oh!

nyurik Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

CommanderStorm Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

nyurik Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

CommanderStorm left a comment

Choose a reason for hiding this comment

Uh oh!

CommanderStorm Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

CommanderStorm Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nuts-rice Sep 19, 2025 •

edited

Loading