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

feat(core): improve PathExt::replace_env and use it throught the codebase #1375

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

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Mar 25, 2025

This new implementation allows for expanding any environment variable so it is not limited to just ~, $HOME, $Env:USERPROFILE and $Env:KOMOREBI_CONFIG_HOME.

It expands the follwing formats:

  • CMD: %variable%
  • PowerShell: $Env:variable
  • Bash: $variable.

Why?

Just for convenience, for example instead of $HOME/AppData/Roaming/applications.json, it can be $APPDATA/applications.json

Remaining questions?

  • Should resolve_home_path function be removed? It is not just a wrapper around the new trait.
  • Should ExpandEnvVars::expand_vars return a result when a variable doesn't exit in env or keep the path as is (current behavior)?

@CtByte
Copy link
Contributor

CtByte commented Mar 25, 2025

I have added this a while back.

My ambition in #1245 was to replace all sorts of path shortcuts in one place. Do you think it could be used?

I would not mind if you remove my extension, just so it all could be in one place

@amrbashir
Copy link
Contributor Author

amrbashir commented Mar 25, 2025

Apologies, I totally didn't see that trait, I should've dug deeper 🙏 .

I think the implementation should be in PathExt as you originally did, However my implementation operates on OsStr instead of lossely converting into a String which could be problematic in some rare cases.

I think I will just move the implementation and tests there.

Is there a reason why your trait was not used instead of resolve_home_path function?

@amrbashir amrbashir changed the title feat(core): add ExpandEnvVars trait to handle environment variable expansion in paths feat(core): improve PathExt::replace_env and use it instead of resolve_home_path Mar 25, 2025
@LGUG2Z
Copy link
Owner

LGUG2Z commented Mar 25, 2025

The long term goal is definitely to make sure what can do what PathExt is doing everywhere we read a Path-like string in every workspace crate - let's target this for v0.1.36 🤞

@LGUG2Z LGUG2Z added the 0.1.36 label Mar 25, 2025
@CtByte
Copy link
Contributor

CtByte commented Mar 25, 2025

Apologies, I totally didn't see that trait, I should've dug deeper 🙏 .

I think the implementation should be in PathExt as you originally did, However my implementation operates on OsStr instead of lossely converting into a String which could be problematic in some rare cases.

I think I will just move the implementation and tests there.

Is there a reason why your trait was not used instead of resolve_home_path function?

I basically only wanted to fix that issue linked in my PR, so I did not want to change everything. I did not forget about it but seems like someone else had the same idea with the paths 😉

Thank you for this PR!

@LGUG2Z
Copy link
Owner

LGUG2Z commented Mar 27, 2025

@amrbashir Can we use this PR to unify the handling across all workspace crates?

@amrbashir
Copy link
Contributor Author

yeah definitely, I will get on it

@amrbashir amrbashir marked this pull request as draft March 28, 2025 16:55
@amrbashir amrbashir changed the title feat(core): improve PathExt::replace_env and use it instead of resolve_home_path feat(core): improve PathExt::replace_env and use it throught the codebase Mar 29, 2025
@amrbashir amrbashir marked this pull request as ready for review March 29, 2025 14:06
@amrbashir
Copy link
Contributor Author

amrbashir commented Mar 29, 2025

@LGUG2Z this should be ready, I searched throughout the code base for path and migrate any code that might need to PathExt::replace_env.

It is possible that I might have missed a few places due to my unfamiliarity with the code base, so if you find any, please let me know.

Most of the paths that needed this trait, are in:

  1. Clap arguments, and that was handled by #[value_parse] attribute and a helper function.
  2. SocketMessage and that was handled by custom deserialization with the help of serde_with crate

That said, I would appreciate a second eye and some extra testing.

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.

3 participants