Add --recursive option #43

DerZade · 2023-02-12T16:18:32Z

Add --recursive option and therefore fix #42.

This PR includes two breaking changes to the public API:

added second parameter (recurse) to spreet::fs::get_svg_input_paths
added second parameter (base_path) to spreet::sprite::sprite_name

I hope this is acceptable. If not, just let me know and I will add other functions instead of extending the existing ones. But this will result in some duplicate code 🙈

flother

This is great, I appreciate you taking the time to add this in. I have a few comments on minor things I'd like to see, but I'll be happy to merge this.

Other than the comments below, could you add a line to CHANGELOG.md to summarise the new flag?

flother · 2023-02-12T20:28:01Z

README.md

@@ -99,6 +99,7 @@ Options:
  -r, --ratio <RATIO>      Set the output pixel ratio [default: 1]
      --retina             Set the pixel ratio to 2 (equivalent to `--ratio=2`)
      --unique             Store only unique images in the spritesheet, and map them to multiple names
+      --recursive          Include images from subdirectories


Suggested change

--recursive Include images from subdirectories

--recursive Include images in sub-directories

Addressed in 7a6a857

flother · 2023-02-12T20:44:02Z

src/fs.rs

-/// (files whose names begin with `.`) but it does follow symlinks.
-pub fn get_svg_input_paths(path: &Path) -> Vec<PathBuf> {
+/// It ignores hidden files (files whose names begin with `.`) but it does follow symlinks.
+pub fn get_svg_input_paths(path: &Path, recurse: bool) -> Vec<PathBuf> {


Let's give this the same name as the command-line argument, for consistency.

Suggested change

pub fn get_svg_input_paths(path: &Path, recurse: bool) -> Vec<PathBuf> {

pub fn get_svg_input_paths(path: &Path, recursive: bool) -> Vec<PathBuf> {

Addressed in 7a6a857

flother · 2023-02-12T20:44:44Z

src/fs.rs

+            if recurse && path_buf.is_dir() {
+                return Some(get_svg_input_paths(&path_buf.as_path(), recurse));


Don't need the borrow, it'll be dereferenced by the compiler

Suggested change

if recurse && path_buf.is_dir() {

return Some(get_svg_input_paths(&path_buf.as_path(), recurse));

if recursive && path_buf.is_dir() {

return Some(get_svg_input_paths(path_buf.as_path(), recursive));

Addressed in 7a6a857

flother · 2023-02-12T20:47:03Z

src/fs.rs

+            if recurse && path_buf.is_dir() {
+                return Some(get_svg_input_paths(&path_buf.as_path(), recurse));
+            }
+


Suggested change

Let's tighten up the whitespace

Addressed in 7a6a857

flother · 2023-02-12T20:47:09Z

src/fs.rs

+            if is_useful_input(&entry) {
+                return Some(vec![path_buf]);
+            }
+


Suggested change

Addressed in 7a6a857

flother · 2023-02-12T21:14:01Z

src/sprite.rs

@@ -228,8 +228,18 @@ impl Spritesheet {
 }

 /// Returns the name (unique id within a spritesheet) taken from a file.
-pub fn sprite_name(path: &Path) -> String {
-    format!("{}", path.file_stem().unwrap().to_string_lossy())
+pub fn sprite_name(path: &Path, base_path: &Path) -> String {


Now it's doing something a little more complicated, I think this function's a perfect candidate for a unit test or two (I'm holding you to a higher standard than I hold myself, sorry!) Do you fancy adding some? One for when base_path has no impact, one when it does

Addressed in 73fda65

flother · 2023-02-12T21:17:37Z

src/sprite.rs

@@ -228,8 +228,18 @@ impl Spritesheet {
 }

 /// Returns the name (unique id within a spritesheet) taken from a file.


Let's add an explanation of what base_path is used for

Suggested change

/// Returns the name (unique id within a spritesheet) taken from a file.

/// Returns the name (unique id within a spritesheet) taken from a file.

///

/// If `base_path` is given, it's used as prefix in the sprite name (minus any parents

/// shared with `path`).

Addressed in 7a6a857

You're explanation starts with "If base_path is given". There is not really a choice tho 😂 so I adjusted it a bit. Is that okay?

DerZade · 2023-02-13T12:05:05Z

Thank you for being so quick with the review :)

I have addressed most of your comments and have added a line to the change log. I have not mentioned any breaking changes to the API because there is no mention of something similar so far. Is that ok, or should I include it in the changelog as well?

~~The only thing left to do is #43 (comment), which I hope to get to tonight :)~~ Done :)

flother

Fantastic! Everything you've done looks good, thanks for responding so quickly.

I realised that cargo fmt/check/clippy run on master but not on PRs (I need to configure the CI workflow properly), so I just ran them now and there's one last change that needs to be made. Sorry I didn't pick this up before, but it's a small change. Clippy's complaining that get_svg_input_paths currently has a needless return. It's an easy fix though, and then we're good to merge.

flother · 2023-02-13T20:22:15Z

src/fs.rs

+
+            if recursive && path_buf.is_dir() {
+                return Some(get_svg_input_paths(path_buf.as_path(), recursive));
+            }
+            if is_useful_input(&entry) {
+                return Some(vec![path_buf]);
+            }
+            return None;


Suggested change

if recursive && path_buf.is_dir() {

return Some(get_svg_input_paths(path_buf.as_path(), recursive));

}

if is_useful_input(&entry) {

return Some(vec![path_buf]);

}

return None;

if recursive && path_buf.is_dir() {

Some(get_svg_input_paths(path_buf.as_path(), recursive))

} else if is_useful_input(&entry) {

Some(vec![path_buf])

} else {

None

}

flother

Brilliant, thanks for taking the time to add this

DerZade · 2023-02-13T21:17:49Z

Thanks a lot for the most pleasant OSS experience I had in a long time :)

Add --recursive option

d732ae5

flother reviewed Feb 12, 2023

View reviewed changes

DerZade added 2 commits February 13, 2023 12:56

Minor tweaks from code reveiw

7a6a857

Add --recursive argument to CHANGELOG.md

462f44c

DerZade requested a review from flother February 13, 2023 12:05

add tests for sprite::sprite_name

73fda65

flother requested changes Feb 13, 2023

View reviewed changes

fix clippy warnings

753bebb

DerZade requested a review from flother February 13, 2023 20:35

flother approved these changes Feb 13, 2023

View reviewed changes

flother merged commit 737f5ed into flother:master Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --recursive option #43

Add --recursive option #43

DerZade commented Feb 12, 2023 •

edited

Loading

flother left a comment

flother Feb 12, 2023

DerZade Feb 13, 2023

flother Feb 12, 2023

DerZade Feb 13, 2023

flother Feb 12, 2023

DerZade Feb 13, 2023

flother Feb 12, 2023

DerZade Feb 13, 2023

flother Feb 12, 2023

DerZade Feb 13, 2023

flother Feb 12, 2023

DerZade Feb 13, 2023

flother Feb 12, 2023

DerZade Feb 13, 2023 •

edited

Loading

DerZade commented Feb 13, 2023 •

edited

Loading

flother left a comment

flother Feb 13, 2023

flother left a comment

DerZade commented Feb 13, 2023

	--recursive Include images from subdirectories
	--recursive Include images in sub-directories

	pub fn get_svg_input_paths(path: &Path, recurse: bool) -> Vec<PathBuf> {
	pub fn get_svg_input_paths(path: &Path, recursive: bool) -> Vec<PathBuf> {

		if recurse && path_buf.is_dir() {
		return Some(get_svg_input_paths(&path_buf.as_path(), recurse));

		@@ -228,8 +228,18 @@ impl Spritesheet {
		}

		/// Returns the name (unique id within a spritesheet) taken from a file.

Add --recursive option #43

Add --recursive option #43

Conversation

DerZade commented Feb 12, 2023 • edited Loading

flother left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerZade Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

DerZade commented Feb 13, 2023 • edited Loading

flother left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flother left a comment

Choose a reason for hiding this comment

DerZade commented Feb 13, 2023

DerZade commented Feb 12, 2023 •

edited

Loading

DerZade Feb 13, 2023 •

edited

Loading

DerZade commented Feb 13, 2023 •

edited

Loading