-
-
Notifications
You must be signed in to change notification settings - Fork 437
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
Icon theme with overrides from config #792
Conversation
Codecov Report
@@ Coverage Diff @@
## master #792 +/- ##
==========================================
+ Coverage 86.57% 86.63% +0.05%
==========================================
Files 44 44
Lines 4507 4527 +20
==========================================
+ Hits 3902 3922 +20
Misses 605 605
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
||
#[derive(Debug, Deserialize, PartialEq, Eq)] | ||
#[serde(rename_all = "kebab-case")] | ||
#[serde(deny_unknown_fields)] | ||
#[serde(default)] | ||
pub struct IconTheme { | ||
pub name: HashMap<String, String>, | ||
pub extension: HashMap<String, String>, | ||
#[serde(deserialize_with = "deserialize_by_name")] |
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.
If we are using a custom deserialize, I think we can avoid creating separate structs.
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 totally agree with you. I updated the PR. (5e81871)
34bc764
to
5e81871
Compare
5e81871
to
4461ffc
Compare
thanks so much, @sudame, it works like a charm |
This PR will close #780
Background
👀 A detailed discussion can be found here: #780
When a user chooses to use custom icons, lsd should apply the custom icons for the conditions specified by the user, and use the default icons for all other conditions.
However, currently lsd ignores the default icons when a user sets custom icons.
Cause
The struct
IconTheme
has HashMap fields namedname
andextension
. The Serde (Rust's deserialization module) can merge default values for structs, but it cannot do so for HashMap. As a result, thename
andextension
fields will not be merged if a custom configuration exists.What I did
ByFilename
struct which wraps a HashMap for thename
andextension
fields.ByFilename
has the IntoIterator trait to minimize the impact on other parts of the code.ByFilename
.Comments
The number of added and removed lines in this PR is significant because I had to move the default icon settings.
TODO
cargo fmt
Update default config/theme in README (if applicable)Update man page at lsd/doc/lsd.md (if applicable)