lsd
                                
                                 lsd copied to clipboard
                                
                                    lsd copied to clipboard
                            
                            
                            
                        Improve theme error informations
I couldn't figure out why my local changes to the theme colors definition were not being picked up, so I went to the source. Turned out my yaml file was using some improper fields.
This PR does:
- print a specific warning if in the config color.themewas set to custom and nocolors.yaml(orcolors.yml) was found
- print a specific warning if a color theme was found, but the parsing failed, and why it failed.
Contrary to before, if a color file definition exists and it cannot be parsed we immediately switch to the default (plus a warning). The previous logic was hiding format errors if there were other file extensions to try.
Some examples of the output (before regular lsd output):
if color.y(a)ml is found, but contains wrong data
Warning: cannot load custom theme. Theme file format invalid. date: unknown field `days-old`, expected one of `hour-old`, `day-old`, `older` at line 18 column 3.
if color.theme is set to "custom" in the config file, but no color.y(a)ml is present
Warning: cannot load custom theme. Cannot read theme file. No such file or directory (os error 2).
Sidenotes:
- warning output bleed into cargo test. This was already happening withWarning: the 'themes' directory is deprecated, use 'colors.yaml' instead., I made it slightly worse. Perhaps in general warnings should be behind#[cfg(not(test))]or similar, didn't investigate inprint_erroreither, considered it out of scope.
- imho we should accept a specific extension for color.yml and the others. Iterating over extensions produces subtle bugs like this and there are no particular benefits (if the documentation tells you to create <filename.yml>, you create <filename.yml> - now you also get a warning)
TODO
- [x] Use cargo fmt
- [x] Add necessary tests
- [ ] Update default config/theme in README (if applicable)
- [ ] Update man page at lsd/doc/lsd.md (if applicable)
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: riquito
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Is there anything I need to do to improve this PR?
hi @riquito, sorry for the late reply, the PR looks good and I have started the CI.
thanks for the contribution!
Codecov Report
Attention: 1 lines in your changes are missing coverage. Please review.
Comparison is base (
89659e4) 85.76% compared to head (cb9930b) 85.87%. Report is 6 commits behind head on master.
| Files | Patch % | Lines | 
|---|---|---|
| src/theme.rs | 96.15% | 1 Missing :warning: | 
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   85.76%   85.87%   +0.11%     
==========================================
  Files          51       51              
  Lines        5001     5026      +25     
==========================================
+ Hits         4289     4316      +27     
+ Misses        712      710       -2     
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.