Add Literals to lots of function arguments

[chrishalcrow]

This PR improves the user experience by adding Literals to LOTS of function arguments (eventually...). They are in a few function arguments at the moment.

Allows for auto-complete for string arguments, like so:

Plan is to go through each module and add Literals everywhere. Will be especially helpful for compute! Before I do so, thought I'd use one concrete example to generate some discussion. I've added Literals for the method argument of detect_bad_channels. To do so, I've done three things:

1. Added the Literal to the typing in the argument list
2. Moved the list of allowed arguments outside the function, just above it. So that we can...
3. Add a test to compare the Literal to the list of allowed arguments

As far I can figure out, Literals have to be hardcoded for static type checkers to pick up on them. To check this hardcoding, I think it's worth adding tests like this for any arguments that might be extended in the future. Any other opinions?

EDIT: The steps 1. 2. 3. might be overkill?? Should I just do 1.?

[zm711]

I think you need a from future import annotations for 3.9 :)

[chrishalcrow]

The "|" notation for types was only added in 3.10 (https://peps.python.org/pep-0604/)!!

So the following code errors on 3.9:

import spikeinterface.full as si
from typing import get_type_hints, get_args
get_type_hints(si.detect_bad_channels)

Does annotations help somehow here, @zm711 ??

This notation is used everywhere in the codebase and 3.14 comes out in 5 months, so I'd vote to skip these tests on python < 3.10?

[zm711]

Yes that's the reason we use that import statement at the top. It allows us to bring in | into 3.9 :)

https://docs.python.org/3/library/__future__.html

[zm711]

Just to be clear it is

from __future__ import annotations

[chrishalcrow]

Hello, thanks! Turns out get_type_hints still doesn't work with annotations from __future__. Instead you need to use __annotations__ to get the types, and un-stringify the output (more: https://blog.derlin.ch/python-type-hints-and-future-annotations). This requires going from

detect_bad_channels_method_literals = get_args(get_type_hints(detect_bad_channels)["method"])

to

from typing import Literal
detect_bad_channels_method_literals = get_args(eval(detect_bad_channels.__annotations__['method']))

(the Literal import is needed for the eval)

[zm711]

Cool, but this becomes moot in 3.10? I think having to run an eval in general is something you want to minimize in general.

Or are you saying the Literal would protect against naughty strings?

[chrishalcrow]

Cool, but this becomes moot in 3.10? I think having to run an eval in general is something you want to minimize in general.

Yeah. detect_bad_channels.__annotations__ returns a string. So alternatively we could make a string_to_args function. Also a bit messy. I'd actually vote to skip the tests if python is < 3.10 and keep the previous nice test method? Then the tests are a lot nicer and we can remove once we change support from 3.10-3.14?

EDIT: Another bad code smell: this method imports Literal but it's only used implicitly.

[zm711]

I guess if we think about it what is the value of this test? This is basically saying that you don't trust the type hints of python and want to confirm they exist? We wouldn't add this test to every file right?

What is it that you want to test?

[chrishalcrow]

I want to protect against a developer adding a new method and forgetting to add it to the typing.

[zm711]

I see so this is just a change detector to annoy Sam :P

[zm711]

So after carefully reading your PR discussion + your edits I think this:

EDIT: The steps 1. 2. 3. might be overkill?? Should I just do 1.?

I think we start with just 1 and then if we decide to add a test later we do that. I'm actually fine with 2 as well because it allows us to easily add in a useful error message (ie you have entered x but only [y,z,a,b] are allowed). But 2 just adds so much work and you're right that if we do 2 then if we want to ensure it is enforced we have to do 3, but then that will cause test failures in refactor potentially. So for me 2 and 3 should really be a 1.0 type move when we are saying the API is 100% stable.

[chrishalcrow]

I came to the same conclusion while trying to get to sleep last night. Importantly, if we miss a method from a Literal, it doesn't mess any of the code up - it just gives the user bad advice via their typing tool