-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Various fixes and improvements #166
Conversation
…without failing the entire test run
Codecov Report
@@ Coverage Diff @@
## master #166 +/- ##
==========================================
+ Coverage 92.62% 94.92% +2.29%
==========================================
Files 16 16
Lines 746 768 +22
Branches 50 65 +15
==========================================
+ Hits 691 729 +38
+ Misses 45 29 -16
Partials 10 10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Switched to try importlib.metadata first (Python 3.8+), then fall back to pkg_resources.get_distribution
I'd rather use setuptools_scm. It determines the package version based on VCS data, and can write a version.py
when building/publishing.
This moves the "determine the current version" logic from runtime into build-time, which makes things a lot less complicated.
Use f-strings where possible, because they're awesome
We should likely use ruff
with the UP
family of rules to auto-apply this (and many other tweaks) for us.
I think that the remaining changes/commits are fine 👍 |
@WhyNotHugo Thanks for your review! I reformatted the usage section a bit and it's looking much cleaner. I like your other suggestions, but I think this PR has already caused enough churn. Instead, I have captured your ideas in issues #167 and #168. Both of those should be fairly straightforward to implement, so perhaps we can get a new contributor to submit PRs for those. I will wait one week or until you give a final 👍 for this before merging it myself (unless I'm told otherwise). |
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.
LGTM. Has conflicts right now.
Fix and improve a many small things.
Description
importlib.metadata
first (Python 3.8+), then fall back topkg_resources.get_distribution
admin.W411
warningpathlib
intests/settings.py
- much more readablewarning.warn
city
andcountry
templatetags, since they might be useful for peoplenow()
in testscontinue-on-error
to GHA steps that test with a-dev
Python version or use Django from themain
branchI noticed that some tests weren't running because we didn't have a valid mmdb database from MaxMind. But, we would need a MaxMind account to download their mmdb files. Additionally, even if we did manage to download those files from MaxMind within tests, we would be testing against real world data that could change out from under us - this actually happened in some of the tests with an IP address that the tests expected to be in San Diego. I wrote a Perl script using MaxMind's mmdb writer Perl library to write minimized mmdb fixture files. I had to reverse engineer the Perl data structures so that the generated files would be properly readable by Python's
geoip2
library, but the result is two very small binary mmdb files that only contain IP addresses we need for tests.Motivation and Context
Silences various warnings during tests, improves GHA test time, modernizes Python syntax, ensures all tests are run in a repeatable manner without relying on third party non-static data, and adds some nice-to-haves.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: