-
Notifications
You must be signed in to change notification settings - Fork 6k
Update pre-commit settings #2415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there. Relax, I am just a little warning for the maintainers to release directly after merging your PR, otherwise we have broken examples and people might get confused :)
|
pre-commit setup worked in a clean venv for me, pylint fails though. |
also the pycharm auto-run thingy? what pylint fails do you get and how do you get them? (i.e. with |
|
I did not use PyCharm's auto run thing, and I'm not 100% certain I know what you are referring to. I used pythons build-in venv module to create a venv inside the cloned repo (checked out the branch ofc), then activated it and ran I will paste the result from pylint in a minute. |
|
long result incoming (python version is 3.9.2 btw): pylint output
|
|
Ah, that's due to pylint not working properly on py 3.8+ iirc. I'll check if buming to v2.7 helps. Otherwise I'll revert to having pre-commit run in it's own venv, where we can choose the python version. that makes pycharm run the hooks before commiting and for me that results in an error message that pops up in the bottom right corner. |
So you want me to check if I get the same error message? |
|
apparently the environments dont install correctly there, I get an error as well: pre-commit output``` 13:41 Commit failed with error 0 file committed, 1 file failed to commit: this is a test [INFO] Installing environment for https://gitlab.com/pycqa/flake8. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... [INFO] Installing environment for https://github.com/asottile/pyupgrade. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... black....................................................................Passed flake8...................................................................Passed pylint...................................................................Failed - hook id: pylint - exit code: 1 |
|
All right, so:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Looks like you edited the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder too keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)
harshil21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some typos.
Btw when I ran pre-commit run -a in this branch locally, I still got this error for mypy:
mypy-ptb.................................................................Failed
- hook id: mypy
- exit code: 1
telegram/ext/utils/webhookhandler.py:126: error: Module has no attribute "WindowsProactorEventLoopPolicy"; maybe "AbstractEventLoopPolicy"? [attr-defined]
Found 1 error in 1 file (checked 136 source files)
Just confirming if you got this too, or is it only me?
|
@harshil21 Thanks! The mypy error you're seeing is due to you not running on Windows, IG. will add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Looks like you edited the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder to keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)
Poolitzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes LGTM

Tries to improve pre-commit settings in order to close #2414 and #2410. A few notes:
message = cast(Message, update.message). IMO that would be too much for the examples, as not everyone actually uses a type checker