Skip to content

Push addToStoreFromDump unsupported(...) down Store class hierarchy#9799

Merged
thufschmitt merged 1 commit intoNixOS:masterfrom
obsidiansystems:push-add-to-store-from-dump-unsupported-down
Jan 18, 2024
Merged

Push addToStoreFromDump unsupported(...) down Store class hierarchy#9799
thufschmitt merged 1 commit intoNixOS:masterfrom
obsidiansystems:push-add-to-store-from-dump-unsupported-down

Conversation

@Ericson2314
Copy link
Member

Motivation

Instead of having it be the default method in Store itself, have it be the implementation in DummyStore and LegacySSHStore. Then just the implementations which fail to provide the method pay the "penalty" of dealing with the icky unimplemented function for non-compliance.

Context

Picks up where #8217. Getting close to no unsupported in the Store interface itself!

More progress on issue #5729.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

…rchy

Instead of having it be the default method in `Store` itself, have it be
the implementation in `DummyStore` and `LegacySSHStore`. Then just the
implementations which fail to provide the method pay the "penalty" of
dealing with the icky `unimplemented` function for non-compliance.

Picks up where NixOS#8217. Getting close to no `unsupported` in the `Store`
interface itself!

More progress on issue NixOS#5729.
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Jan 18, 2024
@thufschmitt
Copy link
Member

Why do that for addToStoreFromDump in particular?

@Ericson2314
Copy link
Member Author

@thufschmitt My goal is to have no unimplemented(.,.) in interfaces, which I think obscures the contract of the interface. Since the only stores that don't have an implementation are:

  1. dummy://: But it will once I use MemorySourceAccessor to give it an in-memory store.
  2. ssh://: This one is already missing many methods, and once Hydra is converted to use ssh-ng:// it can be retired.

So at this point I think it's fair to say this a "required method" that implementations must define. (And if they define it with unimplemented that's their wrongdoing, not the interface's.)

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

That's fair :)
Let's merge then

@thufschmitt thufschmitt merged commit e652322 into NixOS:master Jan 18, 2024
@Ericson2314 Ericson2314 deleted the push-add-to-store-from-dump-unsupported-down branch January 19, 2024 13:26
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Sep 27, 2025
It should be the responsibility of implementations that don't implement
it to say so.

See also PR NixOS#9799, and issue NixOS#5729
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Sep 28, 2025
It should be the responsibility of implementations that don't implement
it to say so.

See also PR NixOS#9799, and issue NixOS#5729
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Sep 30, 2025
It should be the responsibility of implementations that don't implement
it to say so.

See also PR NixOS#9799, and issue NixOS#5729
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Sep 30, 2025
It should be the responsibility of implementations that don't implement
it to say so.

See also PR NixOS#9799, and issue NixOS#5729
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

store Issues and pull requests concerning the Nix store

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants