Split/Refacto main module #253

o2sh · 2020-10-09T22:43:44Z

Did a bit of cleaning/refactoring. Starting with the main.rs module. I extracted the onefetch -h part into cli.rs. I also created an Option struct that holds the input config and pass it along to Info.rs 's constructor.

Next step will be to refactor Info.rs...hopefully before it breaks the 1000 line barrier 😎

spenserblack

LGTM 👍
Just some small notes/suggestions.

spenserblack · 2020-10-12T13:34:32Z

src/main.rs


    if !is_git_installed() {
-        return Err(Error::GitNotInstalled);
+        return Err("Git failed to execute!".into());


Just some syntactic sugar:
I believe return Err(expression.into()); is equivalent to Err(expression)?.

spenserblack · 2020-10-12T13:42:11Z

src/onefetch/cli.rs

+pub struct Options {
+    pub path: String,
+    pub ascii_language: Language,
+    pub ascii_colors: Vec<String>,
+    pub disabled_fields: info_fields::InfoFieldOn,
+    pub no_bold: bool,
+    pub image: Option<DynamicImage>,
+    pub image_backend: Option<Box<dyn image_backends::ImageBackend>>,
+    pub no_merges: bool,
+    pub no_color_blocks: bool,
+    pub number_of_authors: usize,
+    pub excluded: Vec<String>,
+}


Considering CLI options are being represented as a struct, you might want to use the structopt crate, which could handle most of these options. Although that would require a bit of additional refactoring.

I'm not sure this syntax will be able to cover our needs (at least not in one go). Especially for the option's parameters that require post-processing like image-backend and disabled_fields. I could however use this syntax on a simplified Opt struct and then map it to the real Option struct.

Yeah, not saying using structopt is something to do for this PR. Just wanted to bring up the option before I forget it 😆. We may want to use it eventually just to remove the manual definition of all the clap args that then get mapped to this struct.

I could however use this syntax on a simplified Opt struct and then map it to the real Option struct.

Also, the structopt::StructOpt trait can be implemented on a wrapper struct for these fields.

Also, the structopt::StructOpt trait can be implemented on a wrapper struct for these fields.

Great Idea, I may try it in another PR.

spenserblack · 2020-10-12T13:58:34Z

src/onefetch/language.rs

+                    $( Language::$name => include_str!(concat!("../../resources/", $ascii)), )*
+                    Language::Unknown => include_str!("../../resources/unknown.ascii"),


Cargo sets the CARGO_MANIFEST_DIR environment variable, which for this project is the project root.

Suggested change

$( Language::$name => include_str!(concat!("../../resources/", $ascii)), )*

Language::Unknown => include_str!("../../resources/unknown.ascii"),

$( Language::$name => include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/resources/", $ascii)), )*

Language::Unknown => include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/resources/unknown.ascii")),

This way these lines won't have to be updated on future file moves.

spenserblack · 2020-10-12T13:59:20Z

src/onefetch/language.rs

@@ -78,7 +78,7 @@ macro_rules! define_languages {
                    #[test]
                    #[ignore]
                    fn [<$name:lower _width>] () {
-                        const ASCII: &str = include_str!(concat!("../resources/", $ascii));
+                        const ASCII: &str = include_str!(concat!("../../resources/", $ascii));


Suggested change

const ASCII: &str = include_str!(concat!("../../resources/", $ascii));

const ASCII: &str = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/resources/", $ascii));

spenserblack · 2020-10-12T13:59:48Z

src/onefetch/language.rs

@@ -91,7 +91,7 @@ macro_rules! define_languages {
                    #[test]
                    #[ignore]
                    fn [<$name:lower _height>] () {
-                        const ASCII: &str = include_str!(concat!("../resources/", $ascii));
+                        const ASCII: &str = include_str!(concat!("../../resources/", $ascii));


Suggested change

const ASCII: &str = include_str!(concat!("../../resources/", $ascii));

const ASCII: &str = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/resources/", $ascii));

spenserblack · 2020-10-12T14:01:18Z

src/onefetch/license.rs


-static CACHE_DATA: &[u8] = include_bytes!("../resources/licenses/cache.bin.zstd");
+static CACHE_DATA: &[u8] = include_bytes!("../../resources/licenses/cache.bin.zstd");


Suggested change

static CACHE_DATA: &[u8] = include_bytes!("../../resources/licenses/cache.bin.zstd");

static CACHE_DATA: &[u8] = include_bytes!(concat!(env!("CARGO_MANIFEST_DIR"), "/resources/licenses/cache.bin.zstd"));

split/refacto main module

62697c4

o2sh requested a review from spenserblack October 9, 2020 22:43

o2sh added 12 commits October 10, 2020 01:07

add possible values for ascii_colors

038b3c2

refacto declaration of option struct

71787c3

further refactoring of main.rs

2b9f425

try fix build

a2350af

fix error handling

24a3666

use error_chain for error handling

444f3b2

rustfmt

f9e86a0

rearrange files

7fd3b56

fix possible values for -a flag

4d69c11

extract info_fields into its own module

5e595a5

merge Options with cli

8419edd

simplify no_bold logic

be96551

spenserblack approved these changes Oct 12, 2020

View reviewed changes

use CARGO_MANIFEST_DIR instead of relative path

a8f345d

o2sh merged commit a2b0bfd into master Oct 12, 2020

o2sh deleted the refacto-main branch November 4, 2020 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split/Refacto main module #253

Split/Refacto main module #253

o2sh commented Oct 9, 2020 •

edited

Loading

spenserblack left a comment

spenserblack Oct 12, 2020 •

edited

Loading

spenserblack Oct 12, 2020

o2sh Oct 12, 2020

spenserblack Oct 12, 2020

o2sh Oct 12, 2020 •

edited

Loading

spenserblack Oct 12, 2020

spenserblack Oct 12, 2020

spenserblack Oct 12, 2020

spenserblack Oct 12, 2020

		$( Language::$name => include_str!(concat!("../../resources/", $ascii)), )*
		Language::Unknown => include_str!("../../resources/unknown.ascii"),

	const ASCII: &str = include_str!(concat!("../../resources/", $ascii));
	const ASCII: &str = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/resources/", $ascii));


		static CACHE_DATA: &[u8] = include_bytes!("../resources/licenses/cache.bin.zstd");
		static CACHE_DATA: &[u8] = include_bytes!("../../resources/licenses/cache.bin.zstd");

Split/Refacto main module #253

Split/Refacto main module #253

Conversation

o2sh commented Oct 9, 2020 • edited Loading

spenserblack left a comment

Choose a reason for hiding this comment

spenserblack Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

spenserblack Oct 12, 2020

Choose a reason for hiding this comment

o2sh Oct 12, 2020

Choose a reason for hiding this comment

spenserblack Oct 12, 2020

Choose a reason for hiding this comment

o2sh Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

spenserblack Oct 12, 2020

Choose a reason for hiding this comment

spenserblack Oct 12, 2020

Choose a reason for hiding this comment

spenserblack Oct 12, 2020

Choose a reason for hiding this comment

spenserblack Oct 12, 2020

Choose a reason for hiding this comment

o2sh commented Oct 9, 2020 •

edited

Loading

spenserblack Oct 12, 2020 •

edited

Loading

o2sh Oct 12, 2020 •

edited

Loading