domainaware / parsedmarc

A Python package and CLI for parsing aggregate and forensic DMARC reports
https://domainaware.github.io/parsedmarc/
Apache License 2.0
962 stars 210 forks source link

Add typing / drop Python < 3.7 support #458

Closed nhairs closed 3 months ago

nhairs commented 6 months ago

This PR adds Python 3.7+ compatible type annotations (dropping support for <3.7 at the same time).

fixes: #455

At a high level the following changes have been made:

Test Plan

  1. Ensure that flake8 without errors
  2. Ensure that python3 tests.py passes without errors
  3. Ensure that mypy passes without errors.

Open Questions

Q1: Currently I've left all instances of OrderedDict alone as I'm not sure if we're using other features of the class. If we're only using the fact that they keep insertion order we should convert them all to dict since as of 3.7 they are guaranteed to retain insertion order.

That said, I think it would be better to move to using something like Dataclasses or Pydantic to properly type the report objects.

nhairs commented 6 months ago

This is mostly done, just need to cleanup ready for final PR.

CC: @seanthegeek

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 55.15320% with 161 lines in your changes are missing coverage. Please review.

Project coverage is 58.54%. Comparing base (100f12e) to head (e3059bb). Report is 16 commits behind head on master.

:exclamation: Current head e3059bb differs from pull request most recent head 6841201. Consider uploading reports for the commit 6841201 to get more accurate results

Files Patch % Lines
parsedmarc/__init__.py 57.96% 66 Missing :warning:
parsedmarc/mail/graph.py 23.28% 56 Missing :warning:
parsedmarc/mail/gmail.py 28.94% 27 Missing :warning:
parsedmarc/utils.py 85.48% 9 Missing :warning:
parsedmarc/mail/imap.py 57.14% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #458 +/- ## ========================================== + Coverage 58.42% 58.54% +0.11% ========================================== Files 11 11 Lines 1347 1358 +11 ========================================== + Hits 787 795 +8 - Misses 560 563 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nhairs commented 5 months ago

Codecov Report

Attention: 161 lines in your changes are missing coverage. Please review. ....

Looks like this is mostly because there are overall more lines of code so even though it functionally has barely changed, the percentage of lines covered has actually dropped.

I think we can ignore the coverage change in this instance.

seanthegeek commented 3 months ago

I'm not willing to drop Python 3.6 support for a while because RHEL 8 and its derivatives use Python 3.6. The end of life of most popular RHEL 8 derivative, Rocky Linux 8 isn't until May 31, 2029. That said, I'd like to include as much typing information is possible while still maintaining support for RHEL 8 and its derivatives.

nhairs commented 3 months ago

Thanks for reviewing this @seanthegeek :)

Given that postponed annotations (from __future__ import annotations) is only supported in 3.7+, I don't think this diff can easily be made compatible with 3.6 typing (nor am I willing to spend the effort). As a result I'm closing this PR.

It might be worth formalising the supported versions and including / linking to them in the contributing docs.

As an aside: I've decided to continue working on my forked repository to experiment with a new way of running the application. Given that it will not be python 3.6 compatible I don't think it will be possible to merge it (in addition to the many breaking changes). Assuming my experiment is successful I will end completely forking it (including changing the name and releasing it on PyPI). If you wish to discuss this, feel free to reach out to me directly.