Skip to content
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.

Sign up for GitHub

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

Jump to bottom

config: migrate off config.merge() #5015

Merged
merged 7 commits into from
Dec 4, 2024
Merged

config: migrate off config.merge() #5015

merged 7 commits into from
Dec 4, 2024

Conversation

yuja
Copy link
Collaborator

@yuja yuja commented Dec 3, 2024

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@martinvonz
Copy link
Owner

Since the 0.23 release is tomorrow, should we wait with this until after that? This seems like something that could use more than a day of testing. What do people think?

martinvonz
martinvonz reviewed Dec 3, 2024
View reviewed changes
cli/src/config.rs Outdated Show resolved Hide resolved
yuja
yuja commented Dec 3, 2024
View reviewed changes
Copy link
Collaborator Author

@yuja yuja left a comment

Choose a reason for hiding this comment

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

Since the 0.23 release is tomorrow, should we wait with this until after that? This seems like something that could use more than a day of testing.

I'm fine in either way. A bigger change settings.get() -> stacked_config.get() is already in, so this PR has relatively small impact. However, it doesn't provide any UX improvement at this point.

cli/src/config.rs Outdated Show resolved Hide resolved
@martinvonz
Copy link
Owner

I'm fine in either way.

I was thinking that this PR was migrating off the config crate when I asked (before I reviewed it), and then I didn't think to withdraw my question. I'm fine with merging this PR before the release.

@yuja
config: migrate resolved_config_values() to new getter API
cae8d55 
When I wrote the original lookup function, I didn't notice that the root config
value could be accessed as &config.cache without cloning. That's the only reason
I added split_safe_prefix().
@yuja
config: remove unneeded Result wrapping from resolved_config_values()
0a69fb9
@yuja
completion: migrate to UserSettings API
f10e5d7 
config::Config will soon be replaced with StackedConfig, and UserSettings
provides more convenient methods than StackedConfig.
@yuja
completion: remove redundant rebuild of default config
bb41024
@yuja
formatter: migrate tests to be using StackedConfig
32e1a3e
@yuja
cli: migrate remainder of Config::get() to StackedConfig::get()
2ff8750
@yuja
config: remove merge() function which is used only in tests
aa7d7c8
@yuja
Copy link
Collaborator Author

yuja commented Dec 4, 2024

I was thinking that this PR was migrating off the config crate when I asked (before I reviewed it)

That would definitely require testing. It'll also include minor behavior changes.

@yuja yuja merged commit 8748a1e into martinvonz:main Dec 4, 2024
18 checks passed
@yuja yuja deleted the push-xyxwvmuoqnxz branch December 4, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinvonz martinvonz martinvonz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuja @martinvonz