Skip to content
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

Windows tilde expansion and multiple config / theme paths #975

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ofersadan85
Copy link
Contributor

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
Copy link

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

@ChrisDenton
Copy link

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
Copy link
Contributor Author

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

Copy link

muniu-bot bot commented Jan 4, 2024

[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:

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

@ofersadan85
Copy link
Contributor Author

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
Copy link
Contributor

rivy commented Jan 5, 2024

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
Copy link
Contributor Author

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
Copy link
Member

zwpaper commented Jan 8, 2024

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

Copy link
Member

@zwpaper zwpaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ofersadan85 ofersadan85 Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zwpaper
Copy link
Member

zwpaper commented Jan 10, 2024

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

yes, let's leave that enhancement to another PR

Copy link

codecov bot commented Jan 10, 2024

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 zwpaper added this to the v1.1.0 milestone Jan 26, 2024
@zwpaper
Copy link
Member

zwpaper commented Feb 4, 2024

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

@ofersadan85
Copy link
Contributor Author

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
Copy link
Member

zwpaper commented Mar 1, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot lsd ~ on windows Windows support for .config/lsd
5 participants