cleo icon indicating copy to clipboard operation
cleo copied to clipboard

feat(typing): added `py.typed` marker file

Open Secrus opened this issue 3 years ago • 6 comments

Resolves: #74

This PR waits until src/cleo/ui/table module is properly typed.

Secrus avatar Sep 10 '22 22:09 Secrus

While the table module is not typed for now, I think this can be applied anyway, since it's only one module (see #265 ).

Secrus avatar Oct 02 '22 20:10 Secrus

First, even ignoring cleo.ui.table, cleo is not currently clean:

src/cleo/ui/exception_trace.py:270: error: Argument 2 to "_render_exception" of "ExceptionTrace" has incompatible type
"Optional[BaseException]"; expected "Exception"  [arg-type]
                self._render_exception(io, inspector.previous_exception)
                                           ^
Found 1 error in 1 file (checked 71 source files)

(ideally this wants fixing in crashtest I think, this should be a BaseException here and the crashtest.inspector.Inspector should deal in BaseException)

As for cleo.ui.table: it may only be one module, but if the types are bad then that's an imposition on all users of this library. IMO it's bad form to publish types that are unhelpful, which seems to be what this is doing at the moment.

Less bad from a consumer point of view - though still annoying! - would be to remove types from those functions whose types you can't figure out how to fix. That's at least close to being explicit about "we haven't typed this function" rather than publishing types that you know are wrong.

dimbleby avatar Oct 04 '22 18:10 dimbleby

First, even ignoring cleo.ui.table, cleo is not currently clean:

src/cleo/ui/exception_trace.py:270: error: Argument 2 to "_render_exception" of "ExceptionTrace" has incompatible type
"Optional[BaseException]"; expected "Exception"  [arg-type]
                self._render_exception(io, inspector.previous_exception)
                                           ^
Found 1 error in 1 file (checked 71 source files)

(ideally this wants fixing in crashtest I think, this should be a BaseException here and the crashtest.inspector.Inspector should deal in BaseException)

As for cleo.ui.table: it may only be one module, but if the types are bad then that's an imposition on all users of this library. IMO it's bad form to publish types that are unhelpful, which seems to be what this is doing at the moment.

Less bad from a consumer point of view - though still annoying! - would be to remove types from those functions whose types you can't figure out how to fix. That's at least close to being explicit about "we haven't typed this function" rather than publishing types that you know are wrong.

Thanks for your insight, I will look into this type error. As to incomplete typing, I have decided to postpone pushing this and finish the typing game.

Secrus avatar Oct 04 '22 20:10 Secrus

@dimbleby what option did you activate to see this error? mypy doesn't show this

Secrus avatar Oct 04 '22 21:10 Secrus

mypy doesn't show this

well of course it does, I didn't make it up!

At a guess the problem is that you - and the CI pipeline - are only running mypy through precommit. This is inadequate because the pre-commit environment does not contain all the other libraries and type stubs that mypy needs to do proper analysis.

You probably want to shift mypy checking to being a 'proper' step in the CI workflow, cf what poetry does

dimbleby avatar Oct 04 '22 22:10 dimbleby

well of course it does, I didn't make it up!

I wasn't trying to say that you made this up, I simply didn't know how to reproduce this. Shame that mypy doesn't work well with pre-commit. I will change the setup.

Secrus avatar Oct 05 '22 08:10 Secrus

Closing in favor of #274

Secrus avatar Oct 30 '22 23:10 Secrus