Refactor to use os.UserHomeDir() #2740
Conversation
There was a problem hiding this comment.
Thanks, this is a good start!
For the sake of backwards compatibility, however, we should keep the dependency on go-homedir as part of the migration path. In the original issue, I stressed this out:
It's important to note that whenever we add new logic about config file storage, we need to ensure a migration path for users who already have configuration files in the old location.
So, whenever we try to read from a config file, we should check both the new location and the old location, and automatically move the file from old to new location if found. This migratory logic shall stay around until the next major release in the future, when we will remove it.
See #2429 (comment) for more context.
|
Ok so I just made the following changes:
|
internal/config/config_file.go
Outdated
| @@ -30,6 +31,34 @@ func ParseDefaultConfig() (Config, error) { | |||
| return ParseConfig(ConfigFile()) | |||
| } | |||
|
|
|||
| // Get the absolute path for a folder or file. Support for the old homedir method | |||
| // Tied to https://github.com/cli/cli/pull/2740 | |||
| func GetAbsolutePath(elem ...string) (string, error) { | |||
There was a problem hiding this comment.
Could this be named HomeDirPath?
Also, could the name somehow reflect that auto-renaming will happen if the old home location was found? Basically, the idea is that no new callers should ever use this function. It's only to be used in places where we currently relied on mitchellh/go-homedir.
internal/config/config_file.go
Outdated
| fullOldPath := filepath.Join(append([]string{oldHomeDir}, elem...)...) | ||
| fullPath := filepath.Join(append([]string{homeDir}, elem...)...) | ||
|
|
||
| if fullOldPath != fullPath { |
There was a problem hiding this comment.
Could we avoid renaming if elem was empty? This is to have a guard against ever renaming the literal user's home directory, which of course we never want to move/rename.
|
Ok so I changed the following:
|
Part of #2470