Rewrite StoreConfig::getUri in terms of new StoreConfig::getReference#13739
Merged
xokdvium merged 1 commit intoNixOS:masterfrom Aug 13, 2025
Merged
Rewrite StoreConfig::getUri in terms of new StoreConfig::getReference#13739xokdvium merged 1 commit intoNixOS:masterfrom
StoreConfig::getUri in terms of new StoreConfig::getReference#13739xokdvium merged 1 commit intoNixOS:masterfrom
Conversation
Contributor
|
@Ericson2314, CI is failing. Otherwise SGTM |
Member
Author
|
@xokdvium Yeah, its daemon tests. Will need some |
5e6b867 to
ce9079d
Compare
Member
Author
|
Let's see now... |
Contributor
|
Member
Author
|
Oh, there is more of it.... Have to also handle the case where the |
ce9079d to
c06fc09
Compare
Member
Author
|
Now it normalizes the Store URLs in that test, which should work regardless of the daemon version |
Member
xokdvium
reviewed
Aug 13, 2025
d870842 to
5fda299
Compare
…nce` Rather than having store implementations return a free-form URI string, have them return a `StoreReference`. This reflects that fact that this method is supposed to invert `resolveStoreConfig`, which goes from a `StoreReference` to some `StoreConfig` concrete derived class (based on the registry). `StoreConfig::getUri` is kept only as a convenience for the common case that we want to immediately render the `StoreReference`. A few tests were changed to use `local://` not `local`, since `StoreReference` does not encode the `local` and `daemon` shorthands (and instead desugars them to `local://` and `unix://` right away). I think that is fine. `local` and `daemon` still work as input.
5fda299 to
3e7879e
Compare
xokdvium
approved these changes
Aug 13, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Rather than having store implementations return a free-form URI string, have them return a
StoreReference. This reflects that fact that this method is supposed to invertresolveStoreConfig, which goes from aStoreReferenceto someStoreConfigconcrete derived class (based on the registry).Context
StoreConfig::getUriis kept only as a convenience for the common case that we want to immediately render theStoreReference.A few tests were changed to use
local://notlocal, sinceStoreReferencedoes not encode thelocalanddaemonshorthands (and instead desugars them tolocal://andunix://right away). I think that is fine.localanddaemonstill work as input.Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.