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

Added Windows specific config location #154

Merged
merged 2 commits into from May 1, 2023

Conversation

demilich1
Copy link
Contributor

@demilich1 demilich1 commented Apr 27, 2023

Windows by default does not set or use the $HOME environment variable. This commit adds a Windows specific branch which tries to locate the config file in %APPDATA%/erdtree/.erdtreerc instead

Fixes #152

Windows by default does not set or use the $HOME environment variable. This commit adds a Windows specific branch which tries to locate the config file in %APPDATA%/erdtree/.erdtreerc instead
@demilich1
Copy link
Contributor Author

For now I only did the minimal amount of changes required. Couple of remarks:

  1. This adds a new dependency to the dirs crate, I am not sure if you are okay with that. But if we don't use a crate we have to resolve the %APPDATA% path manually, which might be error-prone
  2. Maybe it also makes sense to exclude the $XDG_CONFIG_HOME path when compiling for Windows?

Not related to this feature, but in general when developing on Windows: I could not build the project intially, because of two compile errors:

let out = <str as AnsiEscaped>::truncate(&ln, window_width);
    |                               ^^^^^^^^^^^ use of undeclared type `AnsiEscaped`

After changing AnsiEscaped to just Escaped, everything worked and it compiled. I did not commit this change however, as I was unsure.

Also on Windows at least one unit test fails (not related to my changes, happens when just checking out the base repo). This is the output:

---- dirs_only stdout ----
thread 'dirs_only' panicked at 'assertion failed: `(left == right)`
  left: `"149  B ┌─ the_yellow_king\n447  B ├─ lipsum\n318  B ├─ dream_cycle\n1265 B data\n\n3 directories"`,
 right: `"143  B ┌─ the_yellow_king\n446  B ├─ lipsum\n308  B ├─ dream_cycle\n1241 B data\n\n3 directories"`', tests\dirs_only.rs:7:5

It seems on Windows some byte sizes are different? Not sure what the best way would be to fix this.

@solidiquis
Copy link
Owner

I am in agreement that the dirs crate is the way to go 👍

And yeah let's not compile $XDG_CONFIG_HOME for Windows.

As for the error that you fixed with AnsiEscaped to Escaped thank you. Someone yesterday ran clippy and submitted a PR with some errors that got merged because the entire CI test suite uses Ubuntu so the errors it caused on Windows went unnoticed. Again thank you for fixing that.

As for the failing test let's see what the workflow says once it's done running. I will be on a flight for a bit but am excited to get this merged in!

Added a separate function for resolving the config file on Windows. It no longer checks $XDG_CONFIG_HOME and only looks in $ERDTREE_CONFIG_PATH or %APPDATA%/erdtree
@solidiquis solidiquis merged commit c77b4bc into solidiquis:master May 1, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using config files on Windows requires setting env variables
2 participants