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

Fix broken CI: add pre-commit checks for better CI #896

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

jeking3
Copy link
Contributor

@jeking3 jeking3 commented Oct 13, 2021

It looks like pre-commit was added as a requirement for CI.

This caught a few issues, like having duplicate named test methods, so I fixed those up.

Description

I added a reasonable pre-commit configuration with black, flake8, and reformatting.

Related Issue

This fixes #869

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the make format command to format my code (well, pre-commit...)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read (and updated) the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #896 (0c2457f) into master (83ee3db) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #896   +/-   ##
=======================================
  Coverage   97.83%   97.83%           
=======================================
  Files          22       22           
  Lines        1062     1062           
  Branches      162      162           
=======================================
  Hits         1039     1039           
  Misses         10       10           
  Partials       13       13           
Impacted Files Coverage Δ
simple_history/__init__.py 100.00% <ø> (ø)
simple_history/manager.py 97.67% <ø> (ø)
...delwithcustomattronetoonefield_options_and_more.py 100.00% <ø> (ø)
simple_history/registry_tests/tests.py 96.03% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83ee3db...0c2457f. Read the comment docs.

@jeking3 jeking3 requested review from a team and rossmechanic October 13, 2021 16:27
This was referenced Oct 14, 2021
@jeking3
Copy link
Contributor Author

jeking3 commented Oct 18, 2021

@jazzband/django-simple-history Team, CI has been broken for days. This PR fixes it, please, let's do something to make this project more efficient? All PRs require a review and virtually nobody is doing reviews.

@tim-schilling
Copy link
Contributor

tim-schilling commented Oct 19, 2021 via email

@jeking3
Copy link
Contributor Author

jeking3 commented Oct 19, 2021

Reasoning around splitting comma-separated imports: https://github.com/asottile/reorder_python_imports#why-this-style
I did not disable this as of yet, but I did request a re-review so you would see this.

@tim-schilling
Copy link
Contributor

Thanks for sharing that @jeking3, I now understand the reasoning. I'm still on the fence regarding it, but I'm a curmudgeon.

@jeking3
Copy link
Contributor Author

jeking3 commented Oct 19, 2021

I'll remove that from pre-commit and undo the changes, and the churn should be much less on this PR. What I really want is to get CI running again so we can clear up 4-5 PRs, and hopefully get the as_of PR code reviewed.

@macro1
Copy link
Collaborator

macro1 commented Oct 22, 2021

It looks like adding a blank pre-commit configuration will also get CI running again (such as the one I have in #900). I don't see what added that check to begin with..

@jeking3
Copy link
Contributor Author

jeking3 commented Oct 22, 2021

I removed the utf-8 stamping and import reordering that was previously there.

This exercise did catch two tests identically named, so I fixed that up. A few cases where assert was used instead of self.assertSomething(). Bare asserts are generally to be avoided because they are not guaranteed to run depending on the runtime environment. That was caught by bandit.

@tim-schilling take a look if you have a chance, I think you will find this more palatable.

@jeking3
Copy link
Contributor Author

jeking3 commented Oct 23, 2021

I realized I had left a couple changes in runtests.py from the import reordering and utf-8 stamping before yesterday, so I just fixed that up.

@jeking3
Copy link
Contributor Author

jeking3 commented Oct 25, 2021

@tim-schilling take a look if you have a chance, I think you will find this more palatable.

Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@tim-schilling tim-schilling merged commit 25bb5b2 into jazzband:master Oct 25, 2021
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.

Standardize commit and CI code checks
3 participants