feat: support XDG standards on new installs #230
Conversation
WalkthroughThe changes introduce a new package, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
main.go (1)
381-392: LGTM: Addedmigratecommand for legacy path migration.The addition of the
migratecommand under thedevsubcommands is a good approach for handling the transition to XDG-compliant paths. This aligns well with the PR objective and provides a way for users or developers to migrate existing installations.However, consider the following suggestion:
The successful migration message is logged at the Info level, which might not be visible with the default warn log level. Consider either:
- Logging this message at a higher level (e.g., Warn) to ensure visibility.
- Adding a console output to inform the user of successful migration.
Example:
- log.Info().Msg("migrated legacy paths") + log.Warn().Msg("Successfully migrated legacy paths") + fmt.Println("Legacy paths have been successfully migrated to XDG-compliant locations.")internal/appdirs/appdirs.go (1)
148-152: Ensure parent directories exist inmigrateCacheDirIn
migrateCacheDir, there is no check to create the parent directories of the XDG path before renaming, which could cause theos.Renameoperation to fail if the parent directories do not exist.Add a call to
os.MkdirAllto ensure the parent directories exist:func migrateCacheDir(legacyDir string) error { xdgDir := CacheDirXDG() + // Ensure the destination parent directory exists + err := os.MkdirAll(filepath.Dir(xdgDir), os.ModePerm) + if err != nil { + return err + } - err = os.Rename(legacyDir, xdgDir) + err = os.Rename(legacyDir, xdgDir) if err != nil { log.Error().Err(err).Msg("failed to migrate legacy cache directory") return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- internal/appdirs/appdirs.go (1 hunks)
- main.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
main.go (4)
14-14: LGTM: Import ofappdirspackage.The addition of the
appdirspackage import is correct and necessary for the subsequent usage of its functions in the code.
56-56: LGTM: Updated defaultscaffoldrcpath to use XDG standards.The change to use
appdirs.RCFilepath()as the default value for thescaffoldrcflag aligns well with the PR objective of supporting XDG standards on new installs. This ensures that the configuration file is stored in a standardized location across different platforms.
68-68: LGTM: Updated default cache directory to use XDG standards.The change to use
appdirs.CacheDir()as the default value for thecacheflag is in line with the PR objective of supporting XDG standards on new installs. This ensures that the cache directory is stored in a standardized location across different platforms, improving consistency and adhering to best practices.
Line range hint
1-418: LGTM: Successfully implemented XDG standards support.The changes in this file consistently implement support for XDG standards, which aligns perfectly with the PR objectives. Key points:
- The
appdirspackage is properly imported and utilized.- Default paths for configuration and cache now use XDG-compliant locations.
- A
migratecommand has been added to facilitate the transition for existing installations.These changes improve the application's adherence to cross-platform standards and should enhance user experience across different environments. The implementation is clean and well-integrated into the existing codebase.
| func RCFilepathXDG() string { | ||
| configDir := os.Getenv("XDG_CONFIG_HOME") | ||
| if configDir == "" { | ||
| home, err := os.UserHomeDir() | ||
| if err != nil { | ||
| log.Fatal().Err(err).Msg("failed to get home directory") | ||
| } | ||
|
|
||
| configDir = filepath.Join(home, ".config") | ||
| } | ||
|
|
||
| return filepath.Join(configDir, "scaffold", RCFilename) | ||
| } |
There was a problem hiding this comment.
Avoid using log.Fatal() in library functions
The functions RCFilepathXDG, CacheDirXDG, and homeDir call log.Fatal() when encountering errors. Using log.Fatal() causes the application to exit, which might not be appropriate in a library package, as it prevents the caller from handling errors gracefully.
Consider returning errors instead of exiting, allowing the calling code to decide how to handle them. For example, modify RCFilepathXDG:
-func RCFilepathXDG() string {
+func RCFilepathXDG() (string, error) {
configDir := os.Getenv("XDG_CONFIG_HOME")
if configDir == "" {
home, err := os.UserHomeDir()
if err != nil {
- log.Fatal().Err(err).Msg("failed to get home directory")
+ return "", err
}
configDir = filepath.Join(home, ".config")
}
- return filepath.Join(configDir, "scaffold", RCFilename)
+ return filepath.Join(configDir, "scaffold", RCFilename), nil
}You would need to adjust the calling functions to handle the returned errors appropriately.
Also applies to: 84-95, 97-104
| err = os.Rename(legacyFilepath, xdgFilepath) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("failed to migrate legacy configuration file") | ||
| return err | ||
| } |
There was a problem hiding this comment.
Check for existing destination before renaming
In migrateRCFile and migrateCacheDir, the code attempts to rename the legacy file or directory to the new XDG location without checking if the destination already exists. This can result in an error if the destination file or directory is already present.
Consider checking if the destination exists before attempting to rename:
func migrateRCFile(legacyFilepath string) error {
xdgFilepath := RCFilepathXDG()
+ // Check if the destination file already exists
+ _, err := os.Stat(xdgFilepath)
+ if err == nil {
+ log.Info().Str("file", xdgFilepath).Msg("configuration file already migrated")
+ return nil
+ } else if !os.IsNotExist(err) {
+ return err
+ }
err = os.MkdirAll(filepath.Dir(xdgFilepath), os.ModePerm)
if err != nil {
return err
}
err = os.Rename(legacyFilepath, xdgFilepath)Apply similar checks in migrateCacheDir:
func migrateCacheDir(legacyDir string) error {
xdgDir := CacheDirXDG()
+ // Check if the destination directory already exists
+ _, err := os.Stat(xdgDir)
+ if err == nil {
+ log.Info().Str("directory", xdgDir).Msg("cache directory already migrated")
+ return nil
+ } else if !os.IsNotExist(err) {
+ return err
+ }
+ // Ensure the parent directory exists
+ err = os.MkdirAll(filepath.Dir(xdgDir), os.ModePerm)
+ if err != nil {
+ return err
+ }
err = os.Rename(legacyDir, xdgDir)This ensures that existing files or directories are handled gracefully.
Also applies to: 148-152
internal/appdirs/appdirs.go
Outdated
| legacyFilepath, exists = CacheDirLegacy() | ||
| if exists { | ||
| err := migrateCacheDir(legacyFilepath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent variable naming in MigrateLegacyPaths
In the MigrateLegacyPaths function, the variable legacyFilepath is reused when handling the cache directory, which can be confusing since it's actually a directory path.
Consider renaming the variable to legacyDir when working with directories:
- legacyFilepath, exists = CacheDirLegacy()
+ legacyDir, exists := CacheDirLegacy()
if exists {
- err := migrateCacheDir(legacyFilepath)
+ err := migrateCacheDir(legacyDir)
if err != nil {
return err
}
}This change improves readability and reduces potential confusion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| legacyFilepath, exists = CacheDirLegacy() | |
| if exists { | |
| err := migrateCacheDir(legacyFilepath) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| legacyDir, exists := CacheDirLegacy() | |
| if exists { | |
| err := migrateCacheDir(legacyDir) | |
| if err != nil { | |
| return err | |
| } | |
| } |
| func CacheDirLegacy() (path string, exists bool) { | ||
| dir := homeDir(".scaffold/cache") | ||
| _, err := os.Stat(dir) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
|
|
||
| return dir, true | ||
| } |
There was a problem hiding this comment.
Handle errors precisely in CacheDirLegacy
Similarly, in CacheDirLegacy, all errors from os.Stat are considered as the directory not existing. Important errors might be overlooked.
Apply this diff to improve error handling:
func CacheDirLegacy() (path string, exists bool) {
dir := homeDir(".scaffold/cache")
_, err := os.Stat(dir)
- if err != nil {
+ if err != nil {
+ if os.IsNotExist(err) {
+ return "", false
+ }
+ // Log unexpected errors
+ log.Error().Err(err).Msg("failed to access legacy cache directory")
+ return "", false
}
return dir, true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CacheDirLegacy() (path string, exists bool) { | |
| dir := homeDir(".scaffold/cache") | |
| _, err := os.Stat(dir) | |
| if err != nil { | |
| return "", false | |
| } | |
| return dir, true | |
| } | |
| func CacheDirLegacy() (path string, exists bool) { | |
| dir := homeDir(".scaffold/cache") | |
| _, err := os.Stat(dir) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return "", false | |
| } | |
| // Log unexpected errors | |
| log.Error().Err(err).Msg("failed to access legacy cache directory") | |
| return "", false | |
| } | |
| return dir, true | |
| } |
| func RCFilepathLegacy() (path string, exists bool) { | ||
| fp := homeDir(".scaffold", RCFilename) | ||
| _, err := os.Stat(fp) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
|
|
||
| return fp, true | ||
| } |
There was a problem hiding this comment.
Handle errors precisely in RCFilepathLegacy
In the RCFilepathLegacy function, all errors from os.Stat are treated as the file not existing. This approach may overlook other potential errors (e.g., permission issues) that should be handled differently.
Consider modifying the error handling to specifically check if the error indicates that the file does not exist:
func RCFilepathLegacy() (path string, exists bool) {
fp := homeDir(".scaffold", RCFilename)
_, err := os.Stat(fp)
- if err != nil {
+ if err != nil {
+ if os.IsNotExist(err) {
+ return "", false
+ }
+ // Log unexpected errors
+ log.Error().Err(err).Msg("failed to access legacy configuration file")
+ return "", false
}
return fp, true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func RCFilepathLegacy() (path string, exists bool) { | |
| fp := homeDir(".scaffold", RCFilename) | |
| _, err := os.Stat(fp) | |
| if err != nil { | |
| return "", false | |
| } | |
| return fp, true | |
| } | |
| func RCFilepathLegacy() (path string, exists bool) { | |
| fp := homeDir(".scaffold", RCFilename) | |
| _, err := os.Stat(fp) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| return "", false | |
| } | |
| // Log unexpected errors | |
| log.Error().Err(err).Msg("failed to access legacy configuration file") | |
| return "", false | |
| } | |
| return fp, true | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes