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

Introduce a way to support user defined configurations #142

Merged
merged 5 commits into from
Jan 3, 2025

Conversation

svanharmelen
Copy link
Contributor

This is by no means perfect, but its again taking a few small steps to make minor improvements and introduce some new functionality which I was really missing.

@svanharmelen
Copy link
Contributor Author

Do you need anything from me in order to get this one merged?

@jorgerojas26
Copy link
Owner

Sorry for the delay (christmas). I'll try to review it as soon as i can.

app/config.go Outdated Show resolved Hide resolved
app/config.go Outdated Show resolved Hide resolved
@svanharmelen
Copy link
Contributor Author

Updated as requested. Please note that depending on your OS, this is a breaking change and you will need to move your existing config file to the (potentially) new location.

app/config.go Outdated Show resolved Hide resolved
app/config.go Outdated Show resolved Hide resolved
@jorgerojas26
Copy link
Owner

I really don't like the breaking change. On a fresh install on MacOS, for example, the app does not work out of the box because it can't find the config file. Can we just have default values in case the file is not present?.

Also, i don't like that on macOS the config must be located in "$HOME/Library/Application Support". I'd like to keep supporting "$HOME/.config", can we search on both paths (with $HOME/.config as primary source) or maybe check for $XDG_CONFIG_HOME first?

@svanharmelen
Copy link
Contributor Author

svanharmelen commented Jan 3, 2025

I really don't like the breaking change. On a fresh install on MacOS, for example, the app does not work out of the box because it can't find the config file. Can we just have default values in case the file is not present?.

Good catch! That is not how I intended it to work, of course it should work without having to first create a config file yourself (it indeed uses default values if the config file doesn't exist), so let me update that.

Also, i don't like that on macOS the config must be located in "$HOME/Library/Application Support". I'd like to keep supporting "$HOME/.config", can we search on both paths (with $HOME/.config as primary source) or maybe check for $XDG_CONFIG_HOME first?

OK so what do you suggest? Personally I think adding support for $XDG_CONFIG_HOME makes sense as it seems to be a much used way to set your preferred config dir. Making the app use $HOME/.config if that dir exists is also possible, but feels a bit less logical/expected. In that case I would almost argue to revert this part of the change all together or at least for macOS.

So to prevent multiple additional iterations, please let me know your preference.

@jorgerojas26
Copy link
Owner

I really don't like the breaking change. On a fresh install on MacOS, for example, the app does not work out of the box because it can't find the config file. Can we just have default values in case the file is not present?.

Good catch! That is not how I intended it to work, of course it should work without having to first create a config file yourself (it indeed uses default values if the config file doesn't exist), so let me update that.

Also, i don't like that on macOS the config must be located in "$HOME/Library/Application Support". I'd like to keep supporting "$HOME/.config", can we search on both paths (with $HOME/.config as primary source) or maybe check for $XDG_CONFIG_HOME first?

OK so what do you suggest? Personally I think adding support for $XDG_CONFIG_HOME makes sense as it seems to be a much used way to set your preferred config dir. Making the app use $HOME/.config if that dir exists is also possible, but feels a bit less logical/expected. In that case I would almost argue to revert this part of the change all together or at least for macOS.

So to prevent multiple additional iterations, please let me know your preference.

Thanks for the fix.

Yeah, i think supporting $XDG_CONFIG_HOME is the way.

@svanharmelen
Copy link
Contributor Author

Updated to code to always check for a $XDG_CONFIG_HOME env var (so independent of the OS) and if that isn't set, then we fallback to the os.UserConfigDir function to get the config dir.

@jorgerojas26
Copy link
Owner

Thank you, please fix the merge conflict and we are good to go.

@svanharmelen svanharmelen force-pushed the feat/config branch 3 times, most recently from 2b949a4 to f4dfee1 Compare January 3, 2025 16:10
@svanharmelen svanharmelen marked this pull request as draft January 3, 2025 16:14
@svanharmelen
Copy link
Contributor Author

Converting to a draft for just a minute to verify and double check if everything works as expected. Should only take a few minutes...

@svanharmelen svanharmelen marked this pull request as ready for review January 3, 2025 16:20
@svanharmelen
Copy link
Contributor Author

Did find a minor bug in the pagination logic, but will make a separate PR for that. The code in this PR now seems to work as expected.

@jorgerojas26 jorgerojas26 merged commit 20f3e82 into jorgerojas26:main Jan 3, 2025
2 checks passed
@svanharmelen svanharmelen deleted the feat/config branch January 3, 2025 16:30
@ccoVeille
Copy link
Contributor

Updated to code to always check for a $XDG_CONFIG_HOME env var (so independent of the OS) and if that isn't set, then we fallback to the os.UserConfigDir function to get the config dir.

This remind me a suggestion made here

#142 (comment)

😁😇🤣😂

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.

3 participants