Windows tilde expansion and multiple config / theme paths #975

ofersadan85 · 2024-01-02T11:51:49Z

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 · 2024-01-03T17:01:11Z

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

ChrisDenton · 2024-01-03T17:07:33Z

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 · 2024-01-03T21:35:01Z

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 · 2024-01-04T14:18:23Z

[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 · 2024-01-04T14:19:57Z

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 · 2024-01-05T17:06:21Z

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 · 2024-01-08T09:04:55Z

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 · 2024-01-08T14:54:31Z

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 · 2024-01-10T11:56:08Z

src/config_file.rs

-    #[cfg(not(windows))]
-    pub fn config_file_path() -> Option<PathBuf> {
-        use xdg::BaseDirectories;
-        match BaseDirectories::with_prefix(CONF_DIR) {


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

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

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

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?

@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

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

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

zwpaper · 2024-01-10T12:03:06Z

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 · 2024-01-10T12:05:20Z

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 · 2024-02-04T16:49:30Z

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

ofersadan85 · 2024-02-28T07:45:56Z

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 · 2024-03-01T16:00:57Z

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?

ofersadan85 added 2 commits January 2, 2024 13:36

Add support for tilde (~) expansion on windows

64b155a

Support multiple file paths for config and theme

0f93744

ofersadan85 requested review from meain, Peltoche and zwpaper as code owners January 2, 2024 11:51

muniu-bot bot added the size/L label Jan 2, 2024

fixed "elided_lifetimes_in_associated_constant" warning

9b0edf2

zwpaper requested changes Jan 10, 2024

View reviewed changes

zwpaper added this to the v1.1.0 milestone Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows tilde expansion and multiple config / theme paths #975

Windows tilde expansion and multiple config / theme paths #975

Windows tilde expansion and multiple config / theme paths #975

Are you sure you want to change the base?

Windows tilde expansion and multiple config / theme paths #975

Conversation

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Codecov Report