-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
settings: allow overriding the default for settings #40429
Conversation
Looks fine. I do prefer slightly the simpler solution (that is currently in the branch) that doesn't require changing each of the setting classes. |
3906d51
to
cff567d
Compare
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.
There's two reason why I've done it this way:
- Ergonomics. I think it's much nicer to change the default for a setting only by calling methods on that setting (instead of also having to interact with a
SV
) - Consistency. Dealing with defaults values is already the responsibility of the different
Setting
implementations (thesetToDefault()
methods).
Don't you need to change all setting classes in your case? StringSetting and StateMachineSetting as well?
I don't need to. The Override
method is not part of the Setting
interface, and nor should it be. For StateMachineSetting
in particular, there's no notion of a "default", so there should also not be a notion of overriding it. That's another plus if this patch vs the alternative.
I'll merge this also because it's more ready then the alternative - it's its own commit (which it has to be) and it has a test :p
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
Build failed |
This test was polluting the global Registry which was causing any subsequent test to panic. Release note: None
cff567d
to
611a77d
Compare
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.
failure because of broken/polluting existing test. Fixed in a separate new commit.
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
Build failed |
Release note: None
611a77d
to
71f88c3
Compare
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.
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
Build failed |
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.
failure is some TCL flake, although I can't say what exactly
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
Build succeeded |
Release note: None