Windows tilde expansion and multiple config / theme paths

[ofersadan85]

This PR adds some features that I was really missing in lsd:

• Tilde (~) expansion on Windows - This is already supported when seaching for theme files, and is also supported on windows for many other cli apps. Fixes Cannot lsd ~ on windows #903
• Multiple config paths: accept both config.yml and config.yaml as they are both very common (.yml especially on Windows, as most Windows users are used to 3 letter extensions).
• Multiple config paths (continued) - Support the config file in both $XDG_CONFIG_HOME/lsd or $HOME/.config/lsd on non-Windows platforms, and both %APPDATA%\lsd or %USERPROFILE%\.config\lsd on Windows. This is important for consistency with normal dotfiles for other apps. Fixes Windows support for .config/lsd #758
• Remove dependancy on xdg as this was unneeded. dirs already uses xdg internally, and using only dir is more consistent and reliable for all systems.
• Added support to search theme files in all the same paths that config files are searched for.

TODO

• Use cargo fmt
• Add necessary tests - Not needed, these are really simple changes. All current tests pass
• Update default config/theme in README (if applicable) - Not needed
• Update man page at lsd/doc/lsd.md (if applicable) - not needed. Updated the README.md file only

[Destroy666x]

Any reason to not support XDG env on Windows other than just "consistency" (not followed by bunch of apps like git)?

[ChrisDenton]

There would still need to be a fallback for when xdg environment variables are not set. In that case it makes sense for $XDG_CONFIG_HOME to default to $APPDATA.

[ofersadan85]

Any reason to not support XDG env on Windows other than just "consistency" (not followed by bunch of apps like git)?

The 'xdg' package used before this PR did not support this env variable either way, since it strictly didn't support Windows at all. Removing it didn't change that.

We can now support multiple paths anyway with this PR so more can be added if that's required

[muniu-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ofersadan85

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ofersadan85]

Added a small fix for this compiler warning, as this was also relevant to Windows only:

warning: `&` without an explicit lifetime name cannot be used here
  --> src\meta\filetype.rs:19:34
   |
19 |     const EXECUTABLE_EXTENSIONS: &[&'static str] = &["exe", "msi", "bat", "ps1"];
   |                                  ^
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #115010 <https://github.com/rust-lang/rust/issues/115010>
   = note: `#[warn(elided_lifetimes_in_associated_constant)]` on by default
help: use the `'static` lifetime
   |
19 |     const EXECUTABLE_EXTENSIONS: &'static [&'static str] = &["exe", "msi", "bat", "ps1"];
   |                                   +++++++

[rivy]

Adding XDG support cross-platform for this would be fairly simple.

For all directories in the array [ $XDG_CONFIG_HOME, ...$XDG_CONFIG_DIRS, $HOME/.config, $APPDATA, $LOCALAPPDATA, $USERPROFILE/.config ], if the directory exists and 'lsd' is accessible within it, look for the config file within that 'lsd' subdirectory. If the config file is not found, proceed to the next directory.

The only platform specific issue would likely be the split of the $XDG_CONFIG_DIRS directory list, which is separated by ':' on POSIX and ';' on WinOS.

The described method works as the XDG spec expects and falls back to reasonable defaults on all platforms (including WinOS).

Less platform-specific code which should be a win-win for both devs and users.

[ofersadan85]

Adding XDG support cross-platform for this would be fairly simple.

For all directories in the array [ $XDG_CONFIG_HOME, ...$XDG_CONFIG_DIRS, $HOME/.config, $APPDATA, $LOCALAPPDATA, $USERPROFILE/.config ], if the directory exists and 'lsd' is accessible within it, look for the config file within that 'lsd' subdirectory. If the config file is not found, proceed to the next directory.

The only platform specific issue would likely be the split of the $XDG_CONFIG_DIRS directory list, which is separated by ':' on POSIX and ';' on WinOS.

The described method works as the XDG spec expects and falls back to reasonable defaults on all platforms (including WinOS).

Less platform-specific code which should be a win-win for both devs and users.

I tend to agree with this, but like I said before, this is a bigger change that can wait for after this PR in my opinion. This PR only adds some functionality to reuse code between Windows and other OSs when searching for paths (i.e. using the logic within dirs package) and supporting multiple paths for searching the config. This can later be extended as @zwpaper wants

[zwpaper]

sorry for the late reply, I am in a sick leave, it may need days to get back 🤧

[zwpaper]

this mostly LGTM, thanks so much @ofersadan85.

only a question commented, and I remember there may be some CI compiler issues when upgrading dirs to 5.x, let's run the CI to check it

[zwpaper]

with this change, can we handle the cases where people change the XDG_CONFIG_HOME env?

[ofersadan85]

with this change, can we handle the cases where people change the XDG_CONFIG_HOME env?

The xdg::BseDirectories was never available on Windows, since xdg doesn't support Windows at all. See here

It is possible to just add more possible paths ourselves, now that this can support multiple paths

[ofersadan85]

And internally, dirs already uses this variable anyway, so we are using it too, see here

[zwpaper]

this is used in not windows, according to https://docs.rs/dirs/5.0.1/dirs/fn.config_dir.html, linux will remain the XDG env, but not macOS, it seems like a breaking change for macOS users?

[ofersadan85]

@zwpaper hmm interesting, didn't think of mac :(
Looks like internally dirs also uses this, but I can't test or verify that on my own because I don't have a mac. I'll review the code better to see if that can be corrected

[zwpaper]

it breaks macos: https://github.com/dirs-dev/dirs-rs/blob/main/src/mac.rs#L10C54-L10C54

xdg crate would check the env in macOS

I tested locally

[ofersadan85]

Understood, I'll find a solution to solve this problem and revert back to xdg on any OS that isn't Windows

[zwpaper]

this is a bigger change that can wait for after this PR in my opinion

yes, let's leave that enhancement to another PR

[codecov]

Codecov Report

Attention: Patch coverage is 43.47826% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 84.41%. Comparing base (d2538a6) to head (9b0edf2).
Report is 11 commits behind head on master.

Files	Patch %	Lines
src/config_file.rs	28.57%	10 Missing ⚠️
src/theme.rs	75.00%	2 Missing ⚠️
src/core.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #975      +/-   ##
==========================================
- Coverage   84.51%   84.41%   -0.10%     
==========================================
  Files          51       51              
  Lines        5068     5075       +7     
==========================================
+ Hits         4283     4284       +1     
- Misses        785      791       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zwpaper]

hi @ofersadan85, do you have time for this PR? it seems the CI also needs to be fixed.

[ofersadan85]

hi @ofersadan85, do you have time for this PR? it seems the CI also needs to be fixed.

I apologize again for the delay, I can't find time for this yet. I'll try again soon but feel free to send it off to someone else if it seems more urgent than I can handle

[zwpaper]

hi @ofersadan85, I have rebased your branch, but not able to push to your repo, so I created another PR: #999, should we merge #999 or you pick my change into this one?