Skip to content

Parse a string through from_string, on both types that parse one - #150

Merged
martin-s-a merged 1 commit into
mainfrom
parse-through-from-string
Sep 18, 2026
Merged

martin-s-a merged 1 commit into
mainfrom
parse-through-from-string

Conversation

@martin-s-a

Copy link
Copy Markdown
Contributor

Answers @oierlauzi's question on #148: whether parse_image_location is more
Pythonic as ImageLocation.parse or ImageLocation.from_string, and
consistently with DeviceIndex.

Why a static method and not a constructor

DeviceIndex could take a parsing constructor; ImageLocation cannot.

device_index(std::string_view backend, std::size_t id)                // two
image_location(std::string path, std::size_t position = no_position)  // second defaulted

The one-string signature is free on DeviceIndex, so DeviceIndex("cuda:0")
can only mean "parse". On ImageLocation it is already taken — the
constructor gives a lone string the meaning "this path, whole file" — so a
parsing overload would compete for it, and ImageLocation("3@stack.mrc")
would have two readings with no way to ask for the other. Making both types
parse through a constructor is therefore not on the table; making both parse
through a named method is.

Why from_string and not parse

rexlib documents parse_image_location as "the exact inverse of
to_string"
, and on this side that inverse is visible in the name:

assert ImageLocation.from_string(str(location)) == location

from_* is also the stdlib's spelling for an alternative constructor
(fromisoformat, from_bytes, from_float), where parse as a classmethod
is rarer. That choice settles the failure mode too: every one of those raises,
so from_string raises ValueError and the Optional this returned before
is gone. A caller who wants "parse it, or treat it as a path" writes
try/except, which says so.

What changed on DeviceIndex

.def(py::init(&from_string)) becomes .def_static("from_string", ...). The
message and the ValueError are the ones it already raised; only the spelling
of the entry point moved.

DeviceIndex("cuda:0") stops working. That is a public API break, taken now
because it is as cheap as it will ever be: one call site in the package
(_device.py, behind rexlib.device('cpu')), two tests, version 0.1.0, and
the PyPI upload still if: false. Every other use of DeviceIndex in the
repository is the two-argument form and is untouched.

Also

  • The parse_image_location module function is gone from _binding.em.image
    and from the package's __all__.
  • define_image_location loses its pybind11::module_ parameter, which had
    nothing left to register and would have warned under -Wextra.
  • The parsing tests move into test_image_location.py, beside the type they
    exercise, the way test_device_index.py already keeps both.
  • AGENTS.md records the convention and the reason ImageLocation forces it.

Checked

  • ruff check . passes.
  • The three changed translation units compile clean under
    g++ -std=c++20 -fsyntax-only -Wall -Wextra -Wpedantic.
  • The rejected strings are read off parse_image_location and parse_position
    rather than guessed: empty, index zero, empty index, non-digit index, empty
    path.
  • No reference to parse_image_location survives anywhere.
  • 30 tests collect under tests/em/image/.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@martin-s-a martin-s-a self-assigned this Sep 18, 2026
@martin-s-a martin-s-a added the enhancement New feature or request label Sep 18, 2026
@martin-s-a
martin-s-a merged commit 4ce1c90 into main Sep 18, 2026
36 checks passed
@martin-s-a
martin-s-a deleted the parse-through-from-string branch September 18, 2026 15:26
@martin-s-a martin-s-a linked an issue Sep 18, 2026 that may be closed by this pull request
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.

Bind em::read and em::write

2 participants