Add int type to num_samples on InjectTemplatesRecording.

[JoeZiminski]

Adds a missing case for the num_samples parameter type hint on InjectTemplatesRecording. @zm711 @h-mayorquin was the consensus to change Union and Optional to | in type hints? If so should these be changed in this module?

[h-mayorquin]

Yes, we have been using | instead of Union or Optional. I think is less verbose for Union and way more explicit than Optional.

[zm711]

Also we have switched to all native typing rather than the typing module types. So all the uses of List can be changed to list.

[JoeZiminski]

Great thanks both I'll make some changes to this module then request review

[JoeZiminski]

One consideration, | syntax was introduced in python 3.10. This is okay for docstring and generally won't error (I don't think) with from __future__ import annotations when run on any python version. But if we ever want to use a static type checker then it will not be possible to use it on python <3.10 or add it to pre-commit when running on version < 3.10. However, I see python 3.9 is end of life in october 2025 and I think it's extremely unlikely we will add static type checking to the CI by then. Do you think its ok to proceed?

[zm711]

It'll be the sixth republic before Sam let's us do static type checking. So we've worked under the from __future__ import annotations in so much of the code base that changing this would be a drop in the bucket. I wouldn't worry about it.

[zm711]

numpydoc standard requires a space before the colon after an argument. So the final format is:

arg : float, default: 5.0

we put a space before the first colon since it is required for the linter, but not for the second one. This is Chris's project, but I figure we can help him out a little ;)

[zm711]

Suggested change

	num_samples: list[int] | int | None, default: None
	num_samples : list[int] | int | None, default: None

[h-mayorquin]

Should it not be optional in the case of None?

[JoeZiminski]

As in the default: None part? I'm not sure on this @zm711 may know, happy to make any changes as required

[zm711]

Yeah, this is an issue where we had previously agreed that None would be default: None so much of the code base has been transitioning over to that style. But more recently we had a discussion about using num_samples : list[int] | int | None, optional as an alternative style. Maybe we should add this to a maintenance meeting that way we can all discuss this in person and make sure we are all on the same page with the style. We could do a quick vote to make sure everyone agrees on which of the two styles.

[JoeZiminski]

Nice this sounds good, I think I prefer default: None for consistency and it is clearer than optional if you don't know much about typing. If most of the code is default: None shall I leave this for now and we can discuss at the meeting?

[zm711]

I'm fine with that for now :)

[h-mayorquin]

All right if you find those semantics clearer.

[JoeZiminski]

Thanks both! I've applied changes and made some further changes to docstrings, re-requested review

[zm711]

For these sampling frequency default values I don't know if they should be (in Hz) vs having the (in Hz) down in the actual description. So for me I would prefer:

sampling_frequency : float, default: 30000.0
    The sampling rate entered in Hz. (or samples/second)

opinions @chrishalcrow @h-mayorquin ?

[chrishalcrow]

I'd personally prefer the Hz to be in the description, so that a user can copy and paste the default value and it would work.

[JoeZiminski]

Agree I prefer in the description I will make the change

[zm711]

@JoeZiminski one more discussion point and then a couple of typos :)

[JoeZiminski]

Thanks @zm711! After all that talk of codespell I should actually remember to run it locally 😆

[h-mayorquin]

LGTM