Skip to content
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

Added pre-commit #752

Merged
merged 6 commits into from
Oct 2, 2022
Merged

Added pre-commit #752

merged 6 commits into from
Oct 2, 2022

Conversation

Markus28
Copy link
Collaborator

@Markus28 Markus28 commented Sep 26, 2022

  • This PR adds the checks that are defined in the Makefile as pre-commit hooks.
  • Hopefully, the checks are equivalent to those from the Makefile, but I can't guarantee it.
  • CI remains as it is.
  • As I pointed out on discord, I experienced some conflicts between flake8 and yapf, so it might be better to transition to some other combination (e.g. black).

@Markus28
Copy link
Collaborator Author

Sorry, I ran the wrong python version. It appears mypy isn't happy on Python 3.7. I added the ignore hints back.

@Markus28
Copy link
Collaborator Author

Markus28 commented Oct 1, 2022

I needed to add some new type: ignore because the mypy version changed for CI. This should also fix the CI for other PRs (e.g. the PettingZoo one). It might be a good idea to pin the mypy version.

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2022

Codecov Report

Merging #752 (5b70a5c) into master (65c4e3d) will decrease coverage by 1.11%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
- Coverage   90.53%   89.42%   -1.12%     
==========================================
  Files          70       70              
  Lines        4925     4925              
==========================================
- Hits         4459     4404      -55     
- Misses        466      521      +55     
Flag Coverage Δ
unittests 89.42% <50.00%> (-1.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tianshou/env/worker/subproc.py 57.29% <50.00%> (-29.73%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Trinkle23897
Copy link
Collaborator

Trinkle23897 commented Oct 2, 2022

I'd like to remove mypy in pre-commit because it has inconsistent behaviors with different python version and os, which will block contributors living in non py38+ubuntu2004

@Trinkle23897 Trinkle23897 merged commit b0c8d28 into thu-ml:master Oct 2, 2022
@nuance1979
Copy link
Collaborator

I support the transition from yapf to black. I personally find black more consistent with code formatting.

@Trinkle23897
Copy link
Collaborator

agree

BFAnas pushed a commit to BFAnas/tianshou that referenced this pull request May 5, 2024
- This PR adds the checks that are defined in the Makefile as pre-commit
hooks.
- Hopefully, the checks are equivalent to those from the Makefile, but I
can't guarantee it.
- CI remains as it is.
- As I pointed out on discord, I experienced some conflicts between
flake8 and yapf, so it might be better to transition to some other
combination (e.g. black).
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.

4 participants