Skip to content

fix: allow dynamic storages in ExternalLocation.from_name#313

Draft
lmmx wants to merge 3 commits into
15r10nk:mainfrom
lmmx:dynamic-storage-backends
Draft

fix: allow dynamic storages in ExternalLocation.from_name#313
lmmx wants to merge 3 commits into
15r10nk:mainfrom
lmmx:dynamic-storage-backends

Conversation

@lmmx
Copy link
Copy Markdown
Contributor

@lmmx lmmx commented Oct 21, 2025

1. Dynamic Storage Backends

Description

As discussed in #311 (discussion) I am building a prototype of the phash: storage protocol for
ExternalLocation so we can write external("phash:") and have a file be saved to
content-addressable storage using a perceptual hash rather than a cryptographic one, and this will
effectively serve as deduplication as well as a novel way to tolerate inexact matches in file
content in a snapshot.

To add this we discussed changing the validation from hardocing hash and uuid strings to instead
validate against state().all_storages whose keys would provide the set of possible storage
protocol prefixes.

This PR does so, and changes the error message from

ValueError: storage has to be hash or uuid

to

ValueError: storage 'phash' is not registered. Available: 'hash', 'uuid'

available = ", ".join(map(repr, state().all_storages.keys()))
raise ValueError(
    f"storage '{storage}' is not registered. Available: {available}"
)
  • I comma separate the repr of the string keys rather than the keys themselves to wrap them in single quotes
    which I expect to be clearer even if there are unfamiliar ones.

2. Storage Protocol compare method

As a secondary part of the same work, I also needed a compare method on StorageProtocol to make mine work, which was discussed here

We should also change how == works currently for external snapshots. It currently loads the value from disc and compares it. StorageProtocol needs an API to customize the comparison.

3. Don't increment missing_values in External base class __eq__ method if in create mode

Nudge the increment line to after create so as not to break the test in teardown if it was just created

Related Issue(s)

Checklist

  • I have tested my changes thoroughly (you can download the test coverage with hatch run cov:github).
    • I presume it won't but I have not tried setting up the test suite yet
  • I have added/updated relevant documentation.
    • TODO
  • [x] I have added tests for new functionality (if applicable). N/A
  • I have reviewed my own code for errors.
  • I have added a changelog entry with hatch run changelog:entry
    • TODO
  • I used semantic commits (feat: will cause a major and fix: a minor version bump)
    • TODO: amend commit to fix: and force push overwrite
  • You can squash you commits, otherwise they will be squashed during merge

@lmmx lmmx changed the title feat: allow dynamic storages in ExternalLocation.from_name fix: allow dynamic storages in ExternalLocation.from_name Oct 21, 2025
@lmmx lmmx force-pushed the dynamic-storage-backends branch from ed6c655 to b5cb93c Compare October 22, 2025 22:34
@lmmx
Copy link
Copy Markdown
Contributor Author

lmmx commented Oct 22, 2025

Re-pushed to ensure tests pass incrementally

  • All tests passing on new base commit ce43cde

@lmmx lmmx force-pushed the dynamic-storage-backends branch from b5cb93c to 6002ae4 Compare October 22, 2025 23:03
@lmmx
Copy link
Copy Markdown
Contributor Author

lmmx commented Oct 22, 2025

Re-pushed to reorder commits (protocol may need to be split off if it's the one causing test failures)

There are far fewer test failures on the (now 2nd) commit, the one that moves the missing values

It's possible these failures indicate this cannot be done though

FAILED tests/external/test_external.py::test_outsource - assert 0 == 1
FAILED tests/external/test_external.py::test_show_diff - assert 0 == 1
FAILED tests/external/test_external_file.py::test_external_in_other_dir - assert 0 == 1
FAILED tests/external/test_external_location.py::test_missing_suffix - assert 0 == 1
FAILED tests/external/test_formats.py::test_binary_format - assert 0 == 1
FAILED tests/external/test_formats.py::test_large_binary_format - assert 0 == 1
ERROR tests/external/storage/test_uuid.py::test_test_dir - Failed: some snapshots in this test have incorrect values.
ERROR tests/external/storage/test_uuid.py::test_trim - Failed: some snapshots in this test have incorrect values.
ERROR tests/external/test_external.py::test_tests_dir - Failed: some snapshots in this test have incorrect values.
ERROR tests/external/test_external_file.py::test_external_file - Failed: some snapshots in this test have incorrect values.
ERROR tests/external/test_formats.py::test_replace_format - Failed: some snapshots in this test have incorrect values.
ERROR tests/test_docs.py::test_docs[register_format.md] - Failed: some snapshots in this test have incorrect values.
ERROR tests/test_docs.py::test_docs[external.md] - Failed: some snapshots in this test have incorrect values.

@15r10nk 15r10nk added the enhancement New feature or request label May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants