search

lsd-rs / lsd

The next gen ls command
Apache License 2.0
12.85k stars 420 forks source link

Windows tilde expansion and multiple config / theme paths #975

Open ofersadan85 opened 6 months ago

ofersadan85 commented 6 months ago

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

TODO

Destroy666x commented 6 months ago

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

ChrisDenton commented 6 months ago

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 commented 6 months ago

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[bot] commented 6 months ago

[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](https://github.com/lsd-rs/lsd/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ofersadan85 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

zwpaper commented 6 months ago

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[bot] commented 6 months ago

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 :warning:
src/theme.rs 75.00% 2 Missing :warning:
src/core.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zwpaper commented 5 months ago

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

ofersadan85 commented 4 months ago

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 commented 4 months ago

hi @ofersadan85, I have rebased your branch, but not able to push to your repo, so I created another PR: https://github.com/lsd-rs/lsd/pull/999, should we merge #999 or you pick my change into this one?