Code Hygiene #259

pepoluan · 2021-03-02T13:22:43Z

Depends on #256 ; please merge that first and rebase this to master afterwards.
This PR will remain a draft until then.

What do these changes do?

Activates a lot of flake8 plugins. Then implement a whole bunch of fixes to make them all pass.

This is a partial answer to #128 , although purposefully staying away from outright mypy implementation (because I find some of mypy's demands to be impractical and/or inane).

Are there changes in behavior for the user?

None.

Related issue number

As mentioned, related to #128 although does not close that fully.

Checklist

codecov · 2021-03-02T13:26:18Z

Codecov Report

Merging #259 (641de3d) into master (747a7c4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #259   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1656      1686   +30     
  Branches       306       307    +1     
=========================================
+ Hits          1656      1686   +30

Impacted Files	Coverage Δ
aiosmtpd/__init__.py	`100.00% <100.00%> (ø)`
aiosmtpd/controller.py	`100.00% <100.00%> (ø)`
aiosmtpd/handlers.py	`100.00% <100.00%> (ø)`
aiosmtpd/lmtp.py	`100.00% <100.00%> (ø)`
aiosmtpd/main.py	`100.00% <100.00%> (ø)`
aiosmtpd/proxy_protocol.py	`100.00% <100.00%> (ø)`
aiosmtpd/smtp.py	`100.00% <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 747a7c4...641de3d. Read the comment docs.

pepoluan · 2021-03-02T14:04:16Z

.github/workflows/unit-testing-and-coverage.yml

+          # A bunch of flake8 plugins...
+          grab_f8_plugins=(
+            "from configparser import ConfigParser;"
+            "config = ConfigParser();"
+            "config.read('tox.ini');"
+            "print(config['flake8_plugins']['deps']);"
+          )
+          pip install colorama flake8 $(python -c "${grab_f8_plugins[*]}")


Figured out a way to tox.ini the SSOT for GA 😉

aiosmtpd/__init__.py

pepoluan · 2021-03-02T18:25:49Z

All checks green here and in my local test systems. However, I'm keeping this draft until 1.4.1 gets released.

@warsaw

Selection criteria: * Must be useful, even if many warnings For example: Plugins related to annotations. As @warsaw raised in aio-libs#128, it's a good idea to type-annotate as much as possible. * If not simple, must be maintained "maintained" here is overly-simplified to mean only "master has an update in 2020 or later". Some plugins have not received updates for a long time, but their logic are simple and really does not need updates, so those can also be included on a case-by-case basis. * Will not need significant change in logic Adding annotations, removing temp vars used just for return values, those are not logic changes. So even if lots of instances need to be fixed, that is okay. Plugins that need a restructurization of the logic flow (e.g., "flake8-cognitive-complexity") should not be included; or, if included, should not be activated. At least not until the next Epic.

* Exclude all test modules from ANN001 because there are simply too many test cases to annotate :sweatsmile: * To partially compensate, we put in flake8-annotations-coverage * Finally, activate some leniency for flake8-annotations

As much as possible take care of TAE* and ANN* complaints.

No longer using %-formatting

Because many test modules are now annotated and pulls in some type hints from pytest_mock, and with no pytest-mock install autofixture raises some errors.

After some thinking I don't think this is that useful for aiosmtpd.

For autoprogramm.py, we excluded that from C801 because it's not really our code; we just adopt & adapt it (as allowed per license)

Apparently multiprocessing.Event is "just" a Callable, and pytype balked at that. The class proper is multiprocessing.synchronize.Event, which we import as MP_Event.

Conflicts with the Message handler

For unknown reasons, pytype now balked at importing these two funcs. Because the funcs are simple "shims" anyways, I decided to just inline them.

Too many pragmas to add.

Mostly annotations in the new (Base) Classes. Also one instance of func()-instead-of-compre.

Also allows bytearray.

Because, as the documentation explicitly mentions, aiosmtpd.handlers .Message _is_ an abstract base class and "must be subclassed" Previously, this "abc" status had never been enforced. The code hygiene push applies the abc.ABCMeta to the abstract base classes, resulting in failure of some test cases because they tried to instantiate the abc's.

Note: AST100 warnings (usage of assert) needs to be disabled via project configuration.

pepoluan · 2021-03-22T04:45:30Z

Bogged down due to trying to figure out the b0rkeness of #265

Will rebase & marinate today.

Use the renamed import instead to fully disambiguate between email .message.Message and aiosmtpd.handlers.Message (which has been rename-imported as "AbstractMessageHandler")

pepoluan

Here are some comments for possibly interesting decision points

pepoluan · 2021-03-22T04:55:41Z

aiosmtpd/handlers.py

 import re
 import smtplib
 import sys
 from abc import ABCMeta, abstractmethod
-from email import message_from_bytes, message_from_string
+from argparse import ArgumentParser
+from email.message import Message as Em_Message


Need to rename-import because email.message.Message conflicts with the Message handler defined in this module

pepoluan · 2021-03-22T04:57:40Z

aiosmtpd/handlers.py

-from email import message_from_bytes, message_from_string
+from argparse import ArgumentParser
+from email.message import Message as Em_Message
+from email.parser import BytesParser, Parser


Import these because some validators (pytype? I forgot which exactly) seems can't grok that we're importing message_from_bytes and message_from_string functions. Upon checking their definitions (see below comment) I decided to just import these two classes and copy the wrapper funcs.

pepoluan · 2021-03-22T04:58:47Z

aiosmtpd/handlers.py

+def message_from_bytes(s, *args, **kws):
+    return BytesParser(*args, **kws).parsebytes(s)
+
+
+def message_from_string(s, *args, **kws):
+    return Parser(*args, **kws).parsestr(s)


See previous comment about validators now grokking importation of these two funcs from email.parser. The two funcs are apparently thin wrappers around *Parser(...).parseXXX(...), so I just 'inlined' them here.

pepoluan · 2021-03-22T05:00:03Z

aiosmtpd/proxy_protocol.py

-__all__ = [
+__all__ = ["struct", "partial", "IPv4Address", "IPv6Address"]
+__all__.extend(
    k for k in globals().keys() if k.startswith("V1_") or k.startswith("V2_")
-] + ["struct", "partial", "IPv4Address", "IPv6Address"]
+)


One of the validators puked at that list combining op.

pepoluan · 2021-03-22T05:34:07Z

aiosmtpd/tests/test_handlers.py


 import pytest

 from aiosmtpd.controller import Controller
 from aiosmtpd.handlers import AsyncMessage, Debugging, Mailbox, Proxy, Sink
+from aiosmtpd.handlers import Message as AbstractMessageHandler


Disambiguate with email.message.Message (which actually has been rename-imported to Em_Message, but let's just be very very clear)

pepoluan · 2021-03-22T05:35:26Z

aiosmtpd/tests/test_handlers.py

+class MessageHandler(AbstractMessageHandler):
+    def handle_message(self, message: Em_Message) -> None:
+        pass


because aiosmtpd.handler.Message has been marked as an ABC, we can't use it directly without raising some qa errors. This is a simple concretion of the ABC for use in this test module.

pepoluan · 2021-03-22T05:40:38Z

tox.ini

@@ -10,11 +10,14 @@ envdir =
    py37: {toxworkdir}/3.7
    py38: {toxworkdir}/3.8
    py39: {toxworkdir}/3.9
+    py310: {toxworkdir}/3.10


Early forays into 3.10. This is actually excluded from the envlist, so just for experimental testing purposes.

pepoluan · 2021-03-22T05:42:10Z

tox.ini

+    # Bandit is not needed on diffcov, and seems to be incompatible with 310
+    # So, run only if "not (310 or diffcov)" ==> "(not 310) and (not diffcov)"
+    !py310-!diffcov: bandit -c bandit.yml -r aiosmtpd


In tox logic, the 'and' operator - bounds less tightly than the 'no' operator !

pepoluan · 2021-03-22T05:43:02Z

tox.ini

+[flake8_plugins]
+# This is a pseudo-section that feeds into [testenv:qa] and GA
+# Snippets of letters above these plugins are tests that need to be "select"-ed in flake8 config (in
+# setup.cfg) to activate the respective plugins. If no snippet is given, that means the plugin is
+# always active.
+deps =
+    flake8-bugbear
+    flake8-builtins
+    flake8-coding
+    # C4
+    flake8-comprehensions
+    # JS
+    flake8-multiline-containers
+    # PIE
+    flake8-pie
+    # MOD
+    flake8-printf-formatting
+    # PT
+    flake8-pytest-style
+    # SIM
+    flake8-simplify
+    # Cognitive Complexity looks like a good idea, but to fix the complaints... it will be an epic effort.
+    # So we disable it for now and reenable when we're ready, probably just before 2.0
+    # # CCR
+    # flake8-cognitive-complexity
+    # ECE
+    flake8-expression-complexity
+    # C801
+    flake8-copyright
+    # DUO
+    dlint
+    # TAE
+    flake8-annotations-complexity
+    # TAE
+    flake8-annotations-coverage
+    # ANN
+    flake8-annotations
+    # YTT
+    flake8-2020
+    # N400
+    flake8-broken-line


Extracted into a separate config section for easier reuse. Most notably the extraction code in the GA yml.

pepoluan · 2021-03-22T05:48:44Z

All checks green. Undrafting this and marinate for at least a day.

pepoluan added tech-debt Things that needs to be tidied up to avoid being bitten in the future... documentation labels Mar 2, 2021

pepoluan added this to the 1.5 milestone Mar 2, 2021

pepoluan self-assigned this Mar 2, 2021

pepoluan commented Mar 2, 2021

View reviewed changes

pepoluan mentioned this pull request Mar 2, 2021

Add mypy/static type annotations #128

Open

pepoluan force-pushed the code-hygiene branch from ccd82b0 to 561854c Compare March 4, 2021 08:38

pepoluan mentioned this pull request Mar 5, 2021

Tidying __init__ #261

Open

11 tasks

pepoluan force-pushed the code-hygiene branch 4 times, most recently from 7ff1957 to f0bffca Compare March 8, 2021 14:20

pepoluan added 16 commits March 9, 2021 00:31

Move autostop_loop and AUTOSTOP_DELAY to conftest.py

ea59577

Implement some catchup_delay()s in test_main.py

54a67b4

Bump version to 1.5.0a1 and update NEWS.rst

7701d0e

Tune Annotation Thresholds

a06e955

* Exclude all test modules from ANN001 because there are simply too many test cases to annotate :sweatsmile: * To partially compensate, we put in flake8-annotations-coverage * Finally, activate some leniency for flake8-annotations

Add Annotation

527e0e1

As much as possible take care of TAE* and ANN* complaints.

Solve MOD001 complaints

a2edfb3

No longer using %-formatting

Solve SIM* alerts

9e2ebf3

Add pytest-mock to the deps of "docs"

a58b58d

Because many test modules are now annotated and pulls in some type hints from pytest_mock, and with no pytest-mock install autofixture raises some errors.

Remove the flake8-logging-format plugin

2ede6d6

After some thinking I don't think this is that useful for aiosmtpd.

Fix JS (multiline-containers) complaints

7a3a2b7

Fix C801 (copyright) complaints

080db6a

For autoprogramm.py, we excluded that from C801 because it's not really our code; we just adopt & adapt it (as allowed per license)

Solve DUO (Dlint) complaints

bf064d9

Solve PT (pytest-style) complaints

fa4bbe4

Solve PIE complaints

4511476

Add overlooked annotation

4ca6a3e

pepoluan added 20 commits March 9, 2021 00:51

Use sorted(reverse)

c5eba25

Fix wrong annotation

1cf57b3

Apply black

d8b4732

Change annotation: MP.Event -> MP_Event

39fae59

Apparently multiprocessing.Event is "just" a Callable, and pytype balked at that. The class proper is multiprocessing.synchronize.Event, which we import as MP_Event.

Rename imported Message

c68326f

Conflicts with the Message handler

Change importation of message_from_* funcs

13c82e8

For unknown reasons, pytype now balked at importing these two funcs. Because the funcs are simple "shims" anyways, I decided to just inline them.

Disable SIM106

9e6a4cf

Too many pragmas to add.

Fix post-rebase flake8 complaints

9d08c9e

Mostly annotations in the new (Base) Classes. Also one instance of func()-instead-of-compre.

Add test for Message.prepare_message() coverage

fd5e579

Also allows bytearray.

Update NEWS.rst

664e110

Move flake8 plugins into a pseudo-section

3dfff8f

Make GA grab flake8 plugins from tox

61cbf13

Fix "qa" issues post-rebase

41988f3

Bump Version to 1.5.0a2

cbd0e98

Add qa test for NEWS fragment

8c6ef10

Suppress Python Security warnings

5146d39

Note: AST100 warnings (usage of assert) needs to be disabled via project configuration.

Suppress UnraisableException warning for whole class

0b33f21

Experimentally enable tox-ing on 3.10

53c0aaa

Fix location of tox.ini comment

506f564

pepoluan force-pushed the code-hygiene branch from f0bffca to 506f564 Compare March 22, 2021 04:49

pepoluan added 3 commits March 22, 2021 12:16

Use typing.ByteString instead of custom AnyBytes

571ccd2

Remove double import

1a2e23c

Use the renamed import instead to fully disambiguate between email .message.Message and aiosmtpd.handlers.Message (which has been rename-imported as "AbstractMessageHandler")

Add INTERP for 3.10

641de3d

pepoluan commented Mar 22, 2021

View reviewed changes

pepoluan marked this pull request as ready for review March 22, 2021 05:48

pepoluan merged commit 1a1c1bb into aio-libs:master Mar 23, 2021

pepoluan deleted the code-hygiene branch March 24, 2021 03:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Hygiene #259

Code Hygiene #259

pepoluan commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 2, 2021 •

edited

Loading

pepoluan Mar 2, 2021

pepoluan commented Mar 2, 2021

pepoluan commented Mar 22, 2021

pepoluan left a comment

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan Mar 22, 2021

pepoluan commented Mar 22, 2021

Code Hygiene #259

Code Hygiene #259

Conversation

pepoluan commented Mar 2, 2021 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov bot commented Mar 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

pepoluan commented Mar 2, 2021

pepoluan commented Mar 22, 2021

pepoluan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoluan commented Mar 22, 2021

pepoluan commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 2, 2021 •

edited

Loading