Resolve isomer engine names through the shared registry (item 5, step 3) - #163
Merged
Conversation
BREAKING CHANGE: an unknown isomer engine name raises `ConfigurationError` rather than `ValueError`. The CLI maps it to exit 2 with a hint instead of exit 1 as an unexpected error. Item 5, step 3. The registry added in #162 now has a caller, so it is no longer an unused module on main. `_ENGINE_TYPES` was a tuple; the "unknown engine" message hand-wrote its own list of alternatives; and `available_engines()` restated the tuple. Three statements of one fact. They are one `Registry` now, and the message every backend lookup in the package produces is the same message. Construction stays exactly where it was, in `create`'s if/elif ladder, and this is deliberate rather than unfinished. That ladder's own comment records why: each branch names exactly the arguments its engine takes, so an argument no engine reads cannot survive there unnoticed. A registry that also built the engine would need one signature covering all three -- the keyword bag that property exists to prevent, and the same defect the plan records for `threshold` meaning three different quantities. The registry was designed not to own construction for this reason; this is the first place that pays off. The exception change is a real one and is in the CHANGELOG rather than absorbed as an implementation detail: a caller catching ValueError around `IsomerEngineFactory.create` stops catching it. ConfigurationError is the right type -- a bad engine name is a configuration problem, and it is what every other bad backend name in the package already raises. Suite: 1756 passed, 1 skipped, 70 deselected.
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
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.
Breaking, in one narrow way: an unknown isomer engine name raises
ConfigurationErrorrather thanValueError, so the CLI reports exit 2 with ahint instead of exit 1 as an unexpected error.
The registry added in #162 now has a caller —
mainis no longer carrying anunused module.
Three statements of one fact become one
_ENGINE_TYPESwas a tuple. The "unknown engine" error hand-wrote its own listof alternatives.
available_engines()restated the tuple. Nothing connectedthem.
Construction deliberately stays put
The if/elif ladder in
createis untouched, and that is the design workingrather than the job being half-done. Its own comment records why it exists: each
branch names exactly the arguments its engine takes, so an argument no engine
reads cannot survive there unnoticed.
A registry that also built the engine would need one signature covering all
three backends — the keyword bag that property exists to prevent, and the same
defect the plan records for
thresholdmeaning three different quantities. #162was designed not to own construction for exactly this reason; this is the first
place it pays off.
On the exception change
It is in the CHANGELOG rather than absorbed as an implementation detail: a
caller catching
ValueErroraroundIsomerEngineFactory.createstops catchingit.
ConfigurationErroris the right type — a bad engine name is aconfiguration problem, and it is already what every other bad backend name in
the package raises. The message now also enumerates the alternatives, which the
old one did inconsistently.
Suite: 1756 passed, 1 skipped, 70 deselected.
Still to come
Step 2 — moving
ModelFactoryon — is the larger half, because of thethree-parallel-lists finding recorded in #162:
_adapters(2 entries),available_models()(a hand-written literal with 6), andENGINE_INFO(6),holding different sets and connected by nothing.