fix: don't pass a message when converting an unknown status by name - #2700
Merged
tonyandrewmeyer merged 4 commits intoAug 24, 2026
Merged
Conversation
_EntityStatus.from_status_name('unknown', ...) tried to construct
UnknownStatus(message=...), but UnknownStatus.__init__ takes no
arguments, so any round trip through from_ops() (e.g.
_EntityStatus.from_ops(some_status._to_ops())) raised a TypeError.
Special-case 'unknown' in from_status_name to construct UnknownStatus()
directly, mirroring ops.StatusBase.from_name, which already documents
and implements this same special case: unknown status has no message
to preserve. The other status types all accept a message, so no other
special-casing is needed.
The round-trip test imported _EntityStatus inside the test body; the module already imports the other names it needs from scenario.state, so move it up to join them.
tonyandrewmeyer
marked this pull request as ready for review
August 24, 2026 02:44
james-garner-canonical
left a comment
james-garner-canonical
left a comment
Contributor
There was a problem hiding this comment.
I assume that this from_status_name is public since its exposed on the scenario version of the error status objects. So I wonder if we should take this opportunity to consider making passing a name='unknown', message='not an empty string' an error or warning.
`from_status_name` silently dropped `message` when `name` was 'unknown'. Warn instead, document the behaviour in the docstring, and cover both `from_status_name` and the unknown-with-message case in the tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
james-garner-canonical
approved these changes
Aug 24, 2026
james-garner-canonical
left a comment
james-garner-canonical
left a comment
Contributor
There was a problem hiding this comment.
Good catch here. Thanks for addressing my feedback!
tromai
approved these changes
Aug 24, 2026
tromai
left a comment
tromai
left a comment
Contributor
There was a problem hiding this comment.
Thank you for the fix.
I agree with: warning when unknown status is created with a non-empty message.
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
4 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.
_EntityStatus.from_status_name('unknown', ...) tried to construct UnknownStatus(message=...), but UnknownStatus.init takes no arguments, so any round trip through from_ops() (e.g. _EntityStatus.from_ops(some_status._to_ops())) raised a TypeError.
Special-case 'unknown' in from_status_name to construct UnknownStatus() directly, mirroring ops.StatusBase.from_name, which already documents and implements this same special case: unknown status has no message to preserve. The other status types all accept a message, so no other special-casing is needed.
In Ops you wouldn't create an Unknown status, because you can't set it. But in Scenario you might create one to indicate a starting status.