Skip to content

fix: detect override vs. config-settings#1262

Draft
henryiii wants to merge 1 commit intomainfrom
henryiii/fix/override
Draft

fix: detect override vs. config-settings#1262
henryiii wants to merge 1 commit intomainfrom
henryiii/fix/override

Conversation

@henryiii
Copy link
Copy Markdown
Collaborator

Fix for #1261 generated by copilot in VSCode using auto model selection.

Override-only validation checks the TOML-only snapshot instead of the fully merged settings, which stops config-settings from being misclassified as hard-coded pyproject values.

Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
Comment on lines +159 to +161
if original_value is not None or (
value is not None and full_key not in config_setting_keys
):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the logic here is right, but can't tell exactly what it should be. I think it fails if the user provided if from env-variable.

Comment on lines -262 to +277
static_settings = SourceChain(
self.static_settings = SourceChain(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe what we need here is to break down the self.settings from above to get the EnvSource/ConfSource separate from the *toml_srcs and in the other part check explicitly if the key is in the dynamic_settings.

@henryiii
Copy link
Copy Markdown
Collaborator Author

Yes, this was just directly from a model. I think we either need to ensure env-vars are included, or refactor to separate handling a bit.

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.

2 participants