search

ewjoachim / poetry-to-pre-commit

A pre-commit hook for syncing version numbers between poetry.lock and .pre-commit-config.yaml
MIT License
0 stars 1 forks source link

add missing type hints #12

Closed souliane closed 4 months ago

souliane commented 5 months ago

Successful PR Checklist:

PR label(s):

souliane commented 5 months ago

I overlooked that you already use pyright... so this is duplicating. We can close this or:

  1. keep the type hints that have been added and remove the mypy pre-commit hook. In that case, it would be good to update pyright config so that it does the same as mypy strict mode (for instance, it should complain if some of the type hints that are added by the PR are removed). Also, I think that it should also run on the tests folder.
  2. keep this and remove pyright - I have no strong opinion, that's your call :-)

WDTY?

ewjoachim commented 4 months ago

Also, I think that it should also run on the tests folder.

I'm not convinced about that. I'd say it's likely to make tests harder to write, while not necessarily make them catch more bugs. Do you have a usecase where running mypy on tests results in better code quality ?

keep the type hints that have been added and remove the mypy pre-commit hook.

I'd go for this one.

update pyright config so that it does the same as mypy strict mode

I've tried, and it complains a lot on the Yaml parser func pre_commit_config_roundtrip. Either I'm going to cast a lot, or ignore type, but that's probably going to be ugly. ruamel.yaml seems not very stric-type-compatible.

ewjoachim commented 4 months ago

I've pushed my attempt to https://github.com/ewjoachim/poetry-to-pre-commit/compare/strict-pyright?expand=1

souliane commented 4 months ago

Also, I think that it should also run on the tests folder.

I'm not convinced about that. I'd say it's likely to make tests harder to write, while not necessarily make them catch more bugs. Do you have a usecase where running mypy on tests results in better code quality ?

It might not improve the runtime code, but it makes IMHO the tests easier to read, prevent some pitfalls and enables a better code analysis / autocompletion etc. in your editor.

I've pushed my attempt to https://github.com/ewjoachim/poetry-to-pre-commit/compare/strict-pyright?expand=1

Could you please double check? It seems like you forgot to push your commit, as I only see the one from this PR.

ewjoachim commented 4 months ago

Could you please double check? It seems like you forgot to push your commit, as I only see the one from this PR.

Sorry, I meant https://github.com/ewjoachim/poetry-to-pre-commit/pull/14 and especially https://github.com/ewjoachim/poetry-to-pre-commit/pull/14/files#diff-755fbd773f19725441edb1b9a61dc22f2e846468785c65a39651d2b253f73ba9

souliane commented 4 months ago

Sorry, I meant #14 and especially https://github.com/ewjoachim/poetry-to-pre-commit/pull/14/files#diff-755fbd773f19725441edb1b9a61dc22f2e846468785c65a39651d2b253f73ba9

I don't know how to configure pyright, but it looks good - especially the github integration :-D

ewjoachim commented 4 months ago

Well the point is: if we activate strict mode, we get the errors linked above. Do you see a way to get rid of errors that doesn't include a bunch of # type: ignore ?

souliane commented 4 months ago

Well the point is: if we activate strict mode, we get the errors linked above. Do you see a way to get rid of errors that doesn't include a bunch of # type: ignore ?

The best I can do is here: https://github.com/souliane/poetry-to-pre-commit/commit/c21d5aeb7d13d4d08ec64103dfb37f9b38764114

I also removed mypy from the config files. I don't like to ignore reportUnknownMemberType globally, but this is still better than without strict mode... Another solution would be to ignore it at the top of the concerned file only with:

# pyright: reportUnknownMemberType=false

mypy strict mode actually does not trigger these errors, but if it was, we could ignore it for the ruaml package only with something like this: 

[[tool.mypy.overrides]]
module = ["ruaml"]
disable_error_code = ["import-untyped"]
ewjoachim commented 4 months ago

Ok, I made a PR there #15