-
Notifications
You must be signed in to change notification settings - Fork 8k
fix(settings): Use schema defaults and only save overrides #8045
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
This change refactors the settings system to be aware of the default values defined in the settings schema. Key changes: - A new `getDefaultSettings` function generates a complete default settings object directly from the schema. - The settings merge process now uses these defaults as the base layer, ensuring a consistent starting point. - `saveSettings` is updated to compare settings against the defaults and only write the values that have been explicitly changed. This results in cleaner user and workspace `settings.json` files, as they will no longer be cluttered with default values. It also makes the settings system more robust and predictable.
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.
Summary of Changes
Hello @galz10, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the settings management system to streamline configuration files. The core change involves ensuring that only non-default settings are saved, leading to cleaner and more manageable user and workspace configuration files. This significantly improves the predictability and maintainability of the application's configuration.
Highlights
- Dynamic Default Generation: A new
getDefaultSettings
function programmatically generates a complete default settings object directly from the schema, ensuring defaults are always in sync with the schema definition. - Improved Merge Logic: The settings merge process now uses these generated defaults as the foundational layer, guaranteeing that the application always operates with a full set of predictable default values, even if user or workspace settings files are missing or empty.
- Intelligent Saving: The
saveSettings
function has been enhanced to perform a diff against the default settings before writing to a file. It filters out any setting that matches its default value, ensuring that only explicitly overridden settings are persisted.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request refactors the settings system to only store non-default values, which is a great improvement for keeping configuration files clean and manageable. The implementation is solid, introducing getDefaultSettings
to derive defaults from the schema and updating the merge logic accordingly. I've found one potential issue with how non-default values are detected that could lead to incorrect behavior. My feedback includes a suggestion to make this comparison more robust.
Size Change: +830 B (+0.01%) Total Size: 13.2 MB ℹ️ View Unchanged
|
Code Coverage Summary
CLI Package - Full Text Report
Core Package - Full Text Report
For detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
The `removeDefaultValues` function has been removed from the settings logic to ensure that all explicitly set values in a settings file are preserved, even if they match the schema's default. This change makes the behavior of settings more predictable. If a user or workspace setting is explicitly defined, it will be saved, preventing unexpected behavior where settings could be implicitly removed if they matched the default. Key changes: - Removed the `removeDefaultValues` and the unused `isEqual` functions. - Updated `saveSettings` to write all provided settings directly. - Adjusted tests for `saveSettings` to align with the new behavior. - Added a test case to verify that workspace settings correctly override user settings, even when resetting a value to its default.
The tests for settings loading and merging were previously comparing the merged settings object to a large, hardcoded object. This was brittle and would break whenever new settings were added to the default configuration. This commit refactors these tests to dynamically construct the expected settings object. It uses the getDefaultSettings() function and overrides only the properties that are relevant to each specific test case. This approach makes the tests more resilient to changes in the settings schema, preventing unnecessary churn and maintenance overhead. A minor comment was also updated to improve clarity.
TLDR
This PR refactors the settings system to only save non-default values to user and workspace
settings.json
files. This keeps configuration files clean by only storing explicit user overrides, making them easier to manage.Dive Deeper
This change introduces a more robust settings architecture by making the system fully aware of the default values defined in the
settingsSchema.ts
.getDefaultSettings
function programmatically generates a complete default settings object directly from the schema. This ensures our defaults are always in sync with the schema definition.saveSettings
function has been enhanced to perform a diff against the default settings before writing to a file. It filters out any setting that matches its default value, ensuring that only explicitly overridden settings are persisted.This approach significantly cleans up the
settings.json
files and makes the entire configuration system more predictable and maintainable.Reviewer Test Plan
To validate these changes, you can test the settings save logic:
npm install
.~/.gemini/settings.json
and.gemini/settings.json
).:set
.general.vimMode
totrue
.~/.gemini/settings.json
. It should now only contain the settings that differ from the default (e.g.,{"vimMode": true}
).general.vimMode
back tofalse
(its default value).~/.gemini/settings.json
. The file should now contain an empty object{}
because the setting has been returned to its default state and is no longer written to the file.Testing Matrix