gnu compatiblity fix for case sensitivity handling of suffixes #86
Conversation
…e sensitivity handling
There was a problem hiding this comment.
Thanks for the PR! See #69 for an older attempt at this.
I like this approach, just have a couple issues with it that I noted. With those fixed I think this is good to go.
| @@ -228,10 +255,7 @@ pub struct LsColors { | |||
| /// Whether Indicator::RegularFile falls back to Indicator::Normal | |||
| /// (see <https://github.com/sharkdp/lscolors/issues/48#issuecomment-1582830387>) | |||
| file_normal_fallback: bool, | |||
|
|
|||
| // Note: you might expect to see a `HashMap` for `suffix_mapping` as well, but we need to | |||
| // preserve the exact order of the mapping in order to be consistent with `ls`. | |||
There was a problem hiding this comment.
What happened to this? The new implementation uses only a HashMap so I don't see how it can distinguish between *.gz=01;33:*.tar.gz=01;31: and *.tar.gz=01;31:*.gz=01;33:, which should behave differently. (Perhaps we are missing a test if this wasn't caught.)
There was a problem hiding this comment.
I thought it was looking for the longest match, that's my bad, I'll fix it
There was a problem hiding this comment.
I think I fixed this, fixed the test also
src/lib.rs
Outdated
| fn push(&mut self, suffix: FileNameSuffix, style: Option<Style>) { | ||
| let normalized_suffix = suffix.to_ascii_lowercase(); | ||
| // decides whether new mapping and it's varaints should be treated as case-sensitive | ||
| let is_case_sensitive = self.mappings.iter().any(|(m_suffix, m_style)| { |
There was a problem hiding this comment.
It would be nice to avoid $LS_COLORS
There was a problem hiding this comment.
okay let me try 😀
There was a problem hiding this comment.
I managed to reduce the complexity, now when adding a single entry it doesn't compare with each entry just with its variants, which I guess in real world situations should be tiny compared to $LS_COLORS.
now case sensitivities are solved in a single pass after all the entries are added.
|
If I understand correctly, we agree to close this in favor of #87. Let me know if that's not true. |
hi, this a pr that tries to fix to this issue.