Type annotate CallSpec2

[bluetech]

Extracted from #6717.

[bluetech]

I had to tweak the code here a bit, mypy can't understand getattr. I also think it's better to avoid it in general.

To show that it is safe, I used Literal here and in the source below Metafunc._resolve_arg_value_types.

[nicoddemus]

(Small tip, no need to change)

When I encounter these situations where we have a fixed number of if/elifs and an else clause as an error, I opt to have an explicit else: clause for the last choice:

            if valtype_for_arg == "params":
                self.params[arg] = val
            elif valtype_for_arg == "funcargs":
                self.funcargs[arg] = val
            else:  # pragma: no cover
                assert False, "Unhandled valtype for arg: {}".format(valtype_for_arg)

Becomes:

            if valtype_for_arg == "params":
                self.params[arg] = val
            else:
                assert valtype_for_arg == "funcargs", "Unhandled valtype for arg: {}".format(valtype_for_arg)
                self.funcargs[arg] = val

It is shorter and avoids having to write a pragma: nocover 😁

[bluetech]

Thanks for the suggestion!

There is actually a reason why I prefer the form I wrote -- it allows to perform exhaustiveness checking. Consider for example that a new value is added to the valtype_for_arg Literal type. Then ideally, mypy would raise an error if I didn't remember to add a case for it in the conditional.

For enums, this is already possible, see python/mypy#5818. In my own code I rely on this greatly.

For Literals, this is not yet supported by mypy, but hopefully will be in the future: python/mypy#6366

Regarding to no cover, WDYT about adding assert False to the coverage exclude_lines list?

[blueyed]

raise NotimplementedError()?

[bluetech]

I think an assert is more appropriate than NotimplementedError for this. assert means "internal assumption broken", while NotimplementedError might imply "not implemented intentionally" which is not the case.

[blueyed]

I see it more like "might be implemented later", like you've decribed it yourself ("a new value is added").
But then maybe lets add ^\s*raise AssertionError\b to the exclude list? I think it is good to be explicit here, to not ignore any assert 0, but your assert False might work for this also already.