Skip to content

Fixed type hint for patch_signals#36

Open
nicobako wants to merge 4 commits intostarfederation:developfrom
nicobako:patch-signals-type-hint
Open

Fixed type hint for patch_signals#36
nicobako wants to merge 4 commits intostarfederation:developfrom
nicobako:patch-signals-type-hint

Conversation

@nicobako
Copy link

@nicobako nicobako commented Mar 10, 2026

This is a small change to fix type hints for the patch_signals function.

I believe that, if the signals is a dict, then it needs to be json-encodable.

Since you call json.dumps() on line 154, then it must be one of the basic types.

From the json.dumps docs:

Serialize obj to a JSON formatted str.

If skipkeys is true then dict keys that are not basic types
(str, int, float, bool, None) will be skipped
instead of raising a TypeError.

Thanks for the awesome python package!

@gazpachoking
Copy link
Collaborator

This could be made more specific, but this PR isn't quite right. Signals can also be lists and dicts. I started making a helper for it in the action helper branch, but I haven't gotten back to work on it more. If we create a generic typealias for SignalValue like in that branch, then the proper annotation here would be something like like type SignalDict = dict[str, SignalValue]

SignalValue: TypeAlias = Union[
str,
int,
float,
bool,
dict[str, "SignalValue"],
list["SignalValue"],
None,
]

@nicobako
Copy link
Author

Yeah, you are right, I didn't think of the case where the value could be a list or a dict.

I just updated the PR to use SignalValue in f0b3a9a

@gazpachoking
Copy link
Collaborator

Oh, lol. I thought I only had SignalValue in that unreleased branch. This looks good. I should probably move where SignalValue is defined, but that doesn't really matter for this patch. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants