Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for --ascii-colors and --ascii-input #680

Merged
merged 4 commits into from
Jun 12, 2022
Merged

Conversation

bitbrain-za
Copy link
Contributor

use self.info.ascii_colors instead of empty array.

#552

@o2sh o2sh self-requested a review June 11, 2022 23:09
@@ -38,7 +38,6 @@ impl<W: Write> Printer<W> {
let center_pad = " ".repeat(CENTER_PAD_LENGTH);
let info_str = format!("{}", &self.info);
let mut info_lines = info_str.lines();
let colors: Vec<DynColors> = Vec::new();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get rid of use owo_colors::DynColors; which is now an unused import.

@@ -62,7 +61,11 @@ impl<W: Write> Printer<W> {
);
} else {
let mut logo_lines = if let Some(custom_ascii) = &self.info.config.ascii_input {
AsciiArt::new(custom_ascii, &colors, !self.info.config.no_bold)
AsciiArt::new(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a little harder than that.
Indeed, self.info.ascii_colors's capacity is based on the dominant language definition (look here). As a result, depending on the dominant language, output may differ:

Example 1 (shell has only 1 color):

Screenshot 2022-06-12 at 01 23 25

Example 2 (TypeScript has 2 colors):

Screenshot 2022-06-12 at 01 24 55

Example 3 (c# has more than 3 colors):

Screenshot 2022-06-12 at 01 25 52

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your guidance. I'll put a WIP prefix until I've made the requested changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

This handles your example cases.
Still rather new to rust so any criticism on the approach taken would be much appreciated.

@bitbrain-za bitbrain-za changed the title fix for --ascii-colors and --ascii-input WIP: fix for --ascii-colors and --ascii-input Jun 12, 2022
@bitbrain-za bitbrain-za changed the title WIP: fix for --ascii-colors and --ascii-input fix for --ascii-colors and --ascii-input Jun 12, 2022
src/ui/mod.rs Outdated
@@ -18,18 +18,14 @@ pub fn get_ascii_colors(
dominant_language
};

let colors = language.get_colors(true_color);
let mut colors: Vec<DynColors> = ascii_colors.iter().map(|s| num_to_color(s)).collect();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let mut language_colors: Vec<DynColors> = language.get_colors(true_color);

if ascii_colors.is_empty() {
    language_colors
} else {
    let mut colors: Vec<DynColors> = ascii_colors.iter().map(|s| num_to_color(s)).collect();

    if language_colors.len() > colors.len() {
        let mut missing = language_colors.drain(colors.len()..).collect();
        colors.append(&mut missing);
    }
    colors
}

minor: for better performance --> exit early when ascii_collors is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I've added it.

@o2sh
Copy link
Owner

o2sh commented Jun 12, 2022

Thanks a lot for you contribution @bitbrain-za

It works great 🎉

Screenshot 2022-06-12 at 18 58 45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants