Skip to content

Comments

feat(typing): added py.typed marker file#254

Closed
Secrus wants to merge 2 commits intopython-poetry:mainfrom
Secrus:feat/typing-marker
Closed

feat(typing): added py.typed marker file#254
Secrus wants to merge 2 commits intopython-poetry:mainfrom
Secrus:feat/typing-marker

Conversation

@Secrus
Copy link
Member

@Secrus Secrus commented Sep 10, 2022

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

@Secrus Secrus added this to the 1.0 milestone Sep 12, 2022
@zanieb zanieb mentioned this pull request Sep 15, 2022
@Secrus Secrus marked this pull request as ready for review October 2, 2022 20:14
@Secrus
Copy link
Member Author

Secrus commented Oct 2, 2022

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 Secrus requested a review from branchv October 3, 2022 07:38
@dimbleby
Copy link
Contributor

dimbleby commented Oct 4, 2022

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.

@Secrus
Copy link
Member Author

Secrus commented Oct 4, 2022

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
Copy link
Member Author

Secrus commented Oct 4, 2022

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

@dimbleby
Copy link
Contributor

dimbleby commented Oct 4, 2022

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

@Secrus
Copy link
Member Author

Secrus commented Oct 5, 2022

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
Copy link
Member Author

Secrus commented Oct 30, 2022

Closing in favor of #274

@Secrus Secrus closed this Oct 30, 2022
@Secrus Secrus deleted the feat/typing-marker branch June 4, 2023 10:43
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