-
Notifications
You must be signed in to change notification settings - Fork 11
Add config file support and move previews setting from manifest #281
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
base: main
Are you sure you want to change the base?
Conversation
- Created DnvmConfig.cs with JSON serialization - Config file respects XDG spec on Linux (~/.config/dnvm/) - Moved PreviewsEnabled from manifest to config - Removed --enable-dnvm-previews flag - Updated help text and tests - Migration path from old manifests to config file Co-authored-by: agocke <[email protected]>
- Updated DnvmRunner to pass test config directory via environment variable - Fixed UpdateTests and SelfInstallTests to use test config directory - All tests now passing (157 unit tests, 17 integration tests) Co-authored-by: agocke <[email protected]>
| if (!config.PreviewsEnabled) | ||
| { | ||
| // Migrate the setting to the config file | ||
| DnvmConfigFile.Write(config with { PreviewsEnabled = true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please remove this -- we don't want to support writing config files right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the automatic migration code in commit 9dfc5fe. Users will now need to manually create and edit the config file.
Per feedback, removed the automatic migration code that would write to the config file when reading old manifests. Users will need to manually create/edit the config file instead. Co-authored-by: agocke <[email protected]>
src/dnvm/DnvmConfig.cs
Outdated
| /// Get the platform-specific config directory path. | ||
| /// - Linux: ~/.config/dnvm/ (XDG_CONFIG_HOME/dnvm) | ||
| /// - macOS: ~/Library/Application Support/dnvm/ | ||
| /// - Windows: %LOCALAPPDATA%/dnvm/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Configuration data should not be local by default -- I think this should be AppData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use ApplicationData in commit 8f70456. Config files now use roaming app data: %APPDATA% on Windows and ~/.config on macOS/Linux.
Changed config location to use roaming application data: - Windows: %APPDATA% instead of %LOCALAPPDATA% - macOS: ~/.config instead of ~/Library/Application Support - Linux: unchanged (~/.config) This is more appropriate for configuration files that can be synced across machines. Co-authored-by: agocke <[email protected]>
src/dnvm/DnvmConfig.cs
Outdated
| /// <summary> | ||
| /// Static methods for config file operations. | ||
| /// </summary> | ||
| public static class DnvmConfigFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Let's make this a class with instance methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to use instance methods in commit 8c42896. DnvmConfigFile is now a class that can be instantiated and used with instance methods.
Changed DnvmConfigFile from a static class to a regular class with instance methods. This provides better testability and follows standard OOP patterns. Usage changes: - Before: var config = DnvmConfigFile.Read(); - After: var configFile = new DnvmConfigFile(); var config = configFile.Read(); Co-authored-by: agocke <[email protected]>
Implement config file support and move "use previews" functionality to config file
Summary:
This PR adds a dedicated config file for dnvm settings and migrates the "use previews" functionality from the manifest to this config file. The config file respects platform-specific conventions for configuration storage.
Changes:
~/.config/dnvm/(respects XDG_CONFIG_HOME)~/.config/dnvm/%APPDATA%/dnvm/PreviewsEnabledsetting from manifest to config file--enable-dnvm-previewscommand-line flag (users manually editdnvmConfig.json)PreviewsEnabledfrom config instead of manifestUser Impact:
Users who want to enable preview releases will need to manually create the appropriate config file for their platform and set
"previewsEnabled": true. This provides a cleaner separation and follows platform conventions for roaming configuration data.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.