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

235 truecolor define color #252

Merged
merged 11 commits into from
Oct 20, 2020
Merged

235 truecolor define color #252

merged 11 commits into from
Oct 20, 2020

Conversation

atluft
Copy link
Contributor

@atluft atluft commented Oct 9, 2020

Please consider this change to add true colors mentioned in issue #235

The additional braces are needed in define_languages to change the capture of $colors:expr (a single thing) to a stream of tokens $( $colors:tt )*. The stream of tokens is required for the nested call to define_colors which utilizes the vec! ... token stream as a means of choosing the vector based expansion instead of the struct Colors (a single thing) expansion.

See this excellent explanation of how the parent macro globs an expr into a single thing while tt preserves the token stream.

@spenserblack
Copy link
Collaborator

🤔 no CI checks. @o2sh, should the on: [pull_request] rule be added to the GitHub action?

o2sh added a commit that referenced this pull request Oct 9, 2020
@o2sh
Copy link
Owner

o2sh commented Oct 9, 2020

Absolutely, thanks for the heads up @spenserblack

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Left some notes.

src/language.rs Outdated Show resolved Hide resolved
src/language.rs Outdated Show resolved Hide resolved
src/language.rs Outdated Show resolved Hide resolved
src/language.rs Outdated Show resolved Hide resolved
src/language.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

It would be great if you could add some tests to the macro that

  • enforce that the number of true_colors, if defined, equal the number of basic_colors
  • enforce that there are no TrueColors in the basic_colors field

@atluft
Copy link
Contributor Author

atluft commented Oct 16, 2020

It would be great if you could add some tests to the macro that

  • enforce that the number of true_colors, if defined, equal the number of basic_colors
  • enforce that there are no TrueColors in the basic_colors field

Agree, I can try, what would the appropriate action be? Is panic! correct?

@spenserblack
Copy link
Collaborator

Agree, I can try, what would the appropriate action be?

Just like how the macro defines a mod ascii_size that's conditionally compiled when testing, you can add a mod for testing true colors.

The tests can assert_eq! the lengths. For testing no TrueColors in basic_colors, a panic! might be sufficient.

@atluft
Copy link
Contributor Author

atluft commented Oct 17, 2020

A basic colors failure looks like below. In this simulated case the C language is using a TrueColor instead of Blue.

---- onefetch::language::true_colors::c_basic_color_test stdout ----
thread 'onefetch::language::true_colors::c_basic_color_test' panicked at ' language C has true color Color::TrueColor { r: 0, g: 0, b: 255 } in basic colors at index 1 please change to a basic color: Black, Red, Green, Yellow, Blue, Magenta, Cyan, White, BrightBlack, BrightRed, BrightGreen, BrightYellow, BrightBlue, BrightMagenta, BrightCyan, BrightWhite', src/onefetch/language.rs:182:1

@atluft
Copy link
Contributor Author

atluft commented Oct 17, 2020

A mismatch in array counts for colors looks like below. In this case the swift languag has 10 true colors but only 9 basic colors.

---- onefetch::language::true_colors::swift_color_vector_length_test stdout ----
thread 'onefetch::language::true_colors::swift_color_vector_length_test' panicked at 'assertion failed: `(left == right)`
  left: `9`,
 right: `10`:  left (basic) color length do not match right (true) color length.
Colors {
    basic_colors: vec![
        Color::Red, // 0
        Color::Red, // 1
        Color::Red, // 2
        Color::Red, // 3
        Color::Red, // 4
        Color::Red, // 5
        Color::Red, // 6
        Color::Red, // 7
        Color::Red, // 8
    ],
    true_colors: vec![
        Color::TrueColor { r: 248, g: 129, b: 52 }, // 0
        Color::TrueColor { r: 249, g: 119, b: 50 }, // 1
        Color::TrueColor { r: 249, g: 109, b: 48 }, // 2
        Color::TrueColor { r: 250, g: 99, b: 46 }, // 3
        Color::TrueColor { r: 250, g: 89, b: 44 }, // 4
        Color::TrueColor { r: 251, g: 80, b: 42 }, // 5
        Color::TrueColor { r: 251, g: 70, b: 40 }, // 6
        Color::TrueColor { r: 252, g: 60, b: 38 }, // 7
        Color::TrueColor { r: 252, g: 50, b: 36 }, // 8
        Color::TrueColor { r: 253, g: 40, b: 34 }, // 9
    ],
}
', src/onefetch/language.rs:182:1

@o2sh
Copy link
Owner

o2sh commented Oct 17, 2020

@atluft I moved the is_true_color_terminal() func in cli.rs to make it a config parameter.

src/onefetch/language.rs Outdated Show resolved Hide resolved
src/onefetch/language.rs Outdated Show resolved Hide resolved
src/onefetch/language.rs Outdated Show resolved Hide resolved
src/onefetch/language.rs Outdated Show resolved Hide resolved
src/onefetch/language.rs Outdated Show resolved Hide resolved
Co-authored-by: Spenser Black <spenserblack01@gmail.com>
@atluft
Copy link
Contributor Author

atluft commented Oct 19, 2020

@spenserblack thanks for all your help!

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍
Last step is to fix the merge conflicts 🙂

@o2sh
Copy link
Owner

o2sh commented Oct 19, 2020

Absolutely love this feature by the way 😍

I couldn't wait for it to be merged to start playing with it

Thanks a lot to both of you 👍

@o2sh o2sh merged commit 7f0c08c into o2sh:master Oct 20, 2020
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.

3 participants