-
-
Notifications
You must be signed in to change notification settings - Fork 439
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
Improve theme error informations #947
base: master
Are you sure you want to change the base?
Conversation
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3b35e1f
to
b76c3c2
Compare
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 ReportAttention:
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 ☔ View full report in Codecov by Sentry. |
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