Open riquito opened 9 months ago
[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.
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!
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: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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:
color.theme
was set to custom and nocolors.yaml
(orcolors.yml
) was foundContrary 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
if color.theme is set to "custom" in the config file, but no color.y(a)ml is present
Sidenotes:
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_error
either, considered it out of scope.TODO
cargo fmt