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

Various fixes and improvements #166

Merged
merged 15 commits into from
Oct 5, 2023
Merged

Various fixes and improvements #166

merged 15 commits into from
Oct 5, 2023

Conversation

blag
Copy link
Contributor

@blag blag commented Sep 20, 2023

Fix and improve a many small things.

Description

  • Switched to try importlib.metadata first (Python 3.8+), then fall back to pkg_resources.get_distribution
  • Ignored the new admin.W411 warning
  • Use pathlib in tests/settings.py - much more readable
  • Minimized test matrix in GHA and Tox configs to only test supported pairs of Python/Django
  • Fix a flake8 check regarding warning.warn
  • Use f-strings where possible, because they're awesome
  • Created infrastructure to generate mmdb files for testing, and use them in tests (more on this below)
  • Silence warning during tests about IP address not found in database (silenced it by testing for it)
  • Added city and country templatetags, since they might be useful for people
  • Use a timezone-aware now() in tests
  • Add continue-on-error to GHA steps that test with a -dev Python version or use Django from the main branch

I 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

  • 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:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #166 (840b263) into master (8ebdfe8) will increase coverage by 2.29%.
The diff coverage is 90.74%.

@@            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              
Files Coverage Δ
tests/settings.py 100.00% <100.00%> (ø)
tests/tests.py 100.00% <100.00%> (+1.63%) ⬆️
tests/utils.py 97.05% <100.00%> (ø)
user_sessions/admin.py 100.00% <100.00%> (ø)
user_sessions/templatetags/user_sessions.py 90.27% <66.66%> (+18.61%) ⬆️

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

Copy link
Member

@WhyNotHugo WhyNotHugo left a 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.

tests/settings.py Outdated Show resolved Hide resolved
@WhyNotHugo
Copy link
Member

I think that the remaining changes/commits are fine 👍

This was referenced Oct 5, 2023
@blag
Copy link
Contributor Author

blag commented Oct 5, 2023

@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).

Copy link
Member

@WhyNotHugo WhyNotHugo left a 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.

.github/workflows/test.yml Show resolved Hide resolved
@blag blag merged commit 7415406 into master Oct 5, 2023
38 of 39 checks passed
@blag blag deleted the various-improvements branch October 5, 2023 22:49
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.

2 participants