-
Notifications
You must be signed in to change notification settings - Fork 277
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
Add support for nerd font glyphs in languages info #1395
Conversation
the failed check is an extra space. I didn't think it was worth another commit. |
Looks pretty good so far! If you're planning on pushing up more commits, could you mark this as draft? And then mark it as ready when you're done. |
Do these changes warrant a new test? |
@spenserblack is there anything else i can do for this? Sorry to bother you if not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! One stylistic nitpick, but you don't have to worry about it.
circle_color: DynColors, | ||
icon: &'static str, | ||
} | ||
|
||
fn prepare_languages( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a unit test on this method to cover the case when languages_info.nerd_fonts
is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this a bit easier to test, I'd recommend writing a method that's implemented on either Language
or LanguagesInfo
. This method would resolve the icon or fall back to the circle when it's either disabled or not supported.
For example:
// on language
impl Language {
fn get_chip(&self, use_nerd_fonts: bool) -> &'static str {
if !use_nerd_fonts {
return DEFAULT_ICON;
}
self.get_icon().unwrap_or(DEFAULT_ICON)
}
}
And if you were to implement it on LanguagesInfo
, it would take language
as an argument and use self.nerd_fonts
.
You don't need to use this exact implementation, but this is an example of how you could organize code to make it easier to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a test for this case before I saw your suggestion.
It's identical to the one before it except for the value of nerd_fonts
, and the expected result.
Is it worth changing it?
fn test_prepare_languages_with_nerd_fonts() {
let languages_info = LanguagesInfo {
languages_with_percentage: vec![
LanguageWithPercentage {
language: Language::Go,
percentage: 40_f64,
},
LanguageWithPercentage {
language: Language::Erlang,
percentage: 30_f64,
},
LanguageWithPercentage {
language: Language::Java,
percentage: 20_f64,
},
LanguageWithPercentage {
language: Language::Rust,
percentage: 10_f64,
},
],
true_color: false,
number_of_languages_to_display: 2,
info_color: DynColors::Ansi(AnsiColors::White),
nerd_fonts: true,
};
let color_palette = [
DynColors::Ansi(AnsiColors::Red),
DynColors::Ansi(AnsiColors::Green),
];
let result = prepare_languages(&languages_info, &color_palette);
let expected_result = vec![
LanguageDisplayData {
language: Language::Go.to_string(),
percentage: 40_f64,
chip_color: DynColors::Ansi(AnsiColors::Red),
icon: Language::Go.get_icon().unwrap(),
},
LanguageDisplayData {
language: Language::Erlang.to_string(),
percentage: 30_f64,
chip_color: DynColors::Ansi(AnsiColors::Green),
icon: Language::Erlang.get_icon().unwrap(),
},
LanguageDisplayData {
language: "Other".to_string(),
percentage: 30_f64,
chip_color: DynColors::Ansi(AnsiColors::White),
icon: DEFAULT_ICON,
},
];
assert_eq!(result, expected_result);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied @spenserblack's suggestion in 43023ed
Should be good to go now
Co-authored-by: Ossama Hjaji <ossama-hjaji@live.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very cool new feature! 😍
Thanks a lot for implementing it, @Localghost385. 👍
I left a few suggestions—feel free to take them into consideration when you have time.
BTW, you should probably update the section of CONTRIBUTING.md
that explains how to add support for a new language.
Thanks for implementing this feature, it works really nice! |
That's great news! |
@o2sh I can keep the icons up to date as they are added, it might also be a good idea to have a better place to track missing icons than this pr. |
Sure @Localghost385, you can create a tracking issue and pin it. |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [o2sh/onefetch](https://github.com/o2sh/onefetch) | minor | `2.21.0` -> `2.22.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>o2sh/onefetch (o2sh/onefetch)</summary> ### [`v2.22.0`](https://github.com/o2sh/onefetch/blob/HEAD/CHANGELOG.md#2220-2024-09-20) [Compare Source](o2sh/onefetch@2.21.0...2.22.0) ##### New Features 🎉 - Add support for nerd font glyphs in languages info by [@​Localghost385](https://github.com/Localghost385) in o2sh/onefetch#1395 - \[onefetch.dev] Add nerdfont iconts to the preview by [@​Localghost385](https://github.com/Localghost385) in o2sh/onefetch#1411 - Automate publishing crates to crates.io by [@​musicinmybrain](https://github.com/musicinmybrain) in o2sh/onefetch#1364 ##### Bug Fixes 🐛 - Show future commit dates without panicking by [@​MalteT](https://github.com/MalteT) in o2sh/onefetch#1389 ##### Chores 🧹 - Re-generate the man page with --no-info by [@​musicinmybrain](https://github.com/musicinmybrain) in o2sh/onefetch#1376 - Drop unused shebangs from repo test fixture scripts by [@​musicinmybrain](https://github.com/musicinmybrain) in o2sh/onefetch#1375 ##### Commit Statistics <csr-read-only-do-not-edit/> - 3 commits contributed to the release. - 135 days passed between releases. - 0 commits were understood as [conventional](https://www.conventionalcommits.org). - 3 unique issues were worked on: [#​1389](o2sh/onefetch#1389), [#​1395](o2sh/onefetch#1395), [#​1404](o2sh/onefetch#1404) ##### Commit Details <csr-read-only-do-not-edit/> <details><summary>view details</summary> - **[#​1389](o2sh/onefetch#1389 - Show future commit dates without panicking ([`61ab298`](o2sh/onefetch@61ab298)) - **[#​1395](o2sh/onefetch#1395 - Add support for nerd font glyphs in languages info ([`6bf1807`](o2sh/onefetch@6bf1807)) - **[#​1404](o2sh/onefetch#1404 - Bump gix from 0.64.0 to 0.66.0 in the gix group ([`19f77b2`](o2sh/onefetch@19f77b2)) </details> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Add support for nerd font glyphs in languages info
resolves #1308
far from finalized, but a decent chunk of it is done.
any language missing a special icon just uses the circle one.
I haven't added any test coverage, but all previous tests complete.
Not missing icons
Missing icons