Add Python 3.10 support (close #254) by matus-tomlein · Pull Request #260 · snowplow/snowplow-python-tracker

Matus Tomlein (matus-tomlein) · 2021-12-09T19:49:48Z

This PR addresses issue #254 and adds support for Python 3.10 by dropping the pycontracts library (not updated for some time and incompatible with 3.10 at this time) and adding some other features that require support for Python 2 to be dropped.

The full list of changes is:

Remove pycontracts library that wasn't compatible with Python 3.10
Add Python type hints to all function arguments and return values
Remove support for Python 2 due to the type hints
Add custom contracts (e.g., non_empty_string) evaluated at the start of functions
Run through Flake8 linter and correct style
Add Dockerfile for running tests under all supported Python versions

Note:

Python type hints are not enforced during runtime (they can be checked using mypy static type checking or through IDEs such as PyCharm) so some types of issues that previously raised errors no longer do so
- in particular, this is the case for instance type matching, I didn't implement contracts for checking instance types – I don't think this is a standard behaviour in Python and IDEs should help with these types of errors using the type hints
- I tried to add custom contracts for non-instance-type checks such non-empty strings, values larger than 0, and non-empty lists
There is a potentially breaking change because we no longer raise ContractNotRespected error (from pycontracts) when contracts are violated but the standard ValueError

Coveralls (coveralls) · 2021-12-09T20:10:55Z

Coverage decreased (-0.7%) to 96.791% when pulling 02dbee3 on issue/254-python_310 into 8fc7f1e on master.

Paul Boocock (paulboocock) · 2021-12-09T21:14:47Z

I'm also assigning Ada Tzereme (@adatzer) to review this. I'll hopefully get to it too shortly but I'd love her input on this too given she has been the most recent maintainer on this tracker.

* Remove pycontracts library that wasn't compatible with Python 3.10 * Add Python type hints to all function arguments and return values * Remove support for Python 2 due to the type hints * Add custom contracts (e.g., non_empty_string) evaluated at the start of functions * Run through Flake8 linter and correct style * Remove Python 2 from Github action build

Paul Boocock (paulboocock)

This is looking good to me. I'll let Ada Tzereme (@adatzer) have the final say on approving this PR though.

Just a note that when preparing the 0.10.0 release, we should add a table to the README to make it clear what versions of the tracker support what versions of python (i.e. Python 2.7 is 0.9.x only, Python 3+ is 0.10.x+)

Ada Tzereme (adatzer)

Looks great!! Feels much cleaner and with less dependencies too! Just added few comments below.
Also, just noting this PR targets master, i guess it will go for a release branch eventually, right?
Finally, what do you think of removing the pycontracts's :type function comments too?

Ada Tzereme (adatzer) · 2021-12-13T13:58:15Z


+  # pyenv install 3.10.0
+  if [ ! -e ~/.pyenv/versions/tracker310 ]; then
+    pyenv virtualenv 3.10.0 tracker310


Also here, could we go straight for latest 3.10.1?

You're right, I didn't there was 3.10.1 already, updated!

Ada Tzereme (adatzer) · 2021-12-13T14:07:45Z

        pyVersion = sys.version_info[0]
        if pyVersion < 3:
-            if isinstance(x, basestring):
+            if isinstance(x, str):


This comment refers to lines 165-166: Can we now assume that pyVersion will not be < 3?

True, we don't need that if anymore, removed it.

Ada Tzereme (adatzer) · 2021-12-13T14:56:30Z

+
+class EmitterProtocol(Protocol):
+    def input(self, payload: PayloadDict) -> None:
+        ...


Just to confirm on the ..., is it just used like pass here?

Yes, the ellipsis can have more uses (in slices), but they are commonly used instead of pass. It just looks a bit nicer.

Ada Tzereme (adatzer) · 2021-12-14T06:04:12Z

    strategy:
      matrix:
-        python-version: [2.7, 3.6, 3.7, 3.8, 3.9]
+        python-version: [3.6, 3.7, 3.8, 3.9]


Could we add 3.10 as well for the tests?

Yes, added 3.10 here as well. Just had to make it string, otherwise Github actions understood it as version 3.1.

Ada Tzereme (adatzer) · 2021-12-14T06:30:51Z

+            node_name: FormNodeName,
+            value: Optional[str],
+            type_: Optional[str] = None,
+            element_classes: Optional[Union[List[str], Tuple[str]]] = None,


Also here, for element_classes, the tuple in their previous contract was tuple(str,*). Would that map to Tuple[str, Any]?

Good catch! Changed the tuple to Tuple[str, Any] and moved it to typing.py since it's a bit too long.

Ada Tzereme (adatzer) · 2021-12-14T09:25:59Z

-    def __exit__(self, type, value, traceback):
-        enable_all()
+    def __enter__(self) -> None:
+        disable_contracts


nit - was this meant as a function call? i.e. disable_contracts()

This is a really good catch! It actually means that the ContractsDisabled was useless since all the tests passed without it doing anything. I added the function call and removed ContractsDisabled from it where it wasn't necessary anymore. Also added a couple of tests that make use of it.

Ada Tzereme (adatzer) · 2021-12-14T09:26:56Z

+        disable_contracts
+
+    def __exit__(self, type: Any, value: Any, traceback: Any) -> None:
+        enable_contracts


also here, to call enable_contracts?

Commented above, thanks for this!

Ada Tzereme (adatzer) · 2021-12-14T09:28:40Z

-DEFAULT_PLATFORM = "pc"
-
-new_contract("subject", lambda x: isinstance(x, Subject))
+SupportedPlatform = Literal["pc", "tv", "mob", "cnsl", "iot", "web", "srv", "app"]


nit - would it make sense for this type to be defined in snowplow_tracker.typing as well?

Yes, moved to typing.py.

Ada Tzereme (adatzer) · 2021-12-14T09:33:59Z

+ENV PYENV_ROOT $HOME/.pyenv
+ENV PATH $PYENV_ROOT/shims:$PYENV_ROOT/bin:$PATH
+
+RUN pyenv install 3.5.10 && pyenv install 3.6.14 && pyenv install 3.7.11 && pyenv install 3.8.11 && pyenv install 3.9.6 && pyenv install 3.10.0


could we also go straight for 3.10.1 ?

Yes, updated to 3.10.1.

Ada Tzereme (adatzer) · 2021-12-14T10:12:59Z

-
-new_contract("function", lambda x: hasattr(x, "__call__"))
+SuccessCallback = Callable[[PayloadDictList], None]
+FailureCallback = Callable[[int, PayloadDictList], None]


nit - would it make sense for these types to be defined in snowplow_tracker.typing as well?

Right, moved all of these types to typing.py.

Matus Tomlein (matus-tomlein) · 2021-12-15T13:09:55Z

Thanks a lot Ada Tzereme (@adatzer) for the thorough review! Those were really useful comments and also uncovered a few important problems.

Not sure about removing the :type pycontracts comments – they still have some value in documenting the types although now the type documentation is duplicated in the type hints too... Do you know if they are used when generating any API docs?

Ada Tzereme (adatzer)

Really happy to approve from my side!
Concerning the pycontracts' comments, they are not used to generate API docs as far as i know. They do indeed add some value, so no objection from me for them to stay.

* Fix pycontracts incompatibility with pyparsing v3 (closes snowplow#255) * Update python versions in run-tests script (closes snowplow#256) * Prepare for release * Add Python 3.10 support (close snowplow#254) PR snowplow#260 * Remove pycontracts library that wasn't compatible with Python 3.10 * Add Python type hints to all function arguments and return values * Remove support for Python 2 due to the type hints * Add custom contracts (e.g., non_empty_string) evaluated at the start of functions * Run through Flake8 linter and correct style * Remove Python 2 from Github action build * Add Dockerfile for running tests under all supported Python versions * Add configurable timeout for HTTP requests (close snowplow#258) PR snowplow#259 * Prepare for release * Increment version Co-authored-by: adatzer <ada@snowplowanalytics.com> Co-authored-by: Matúš Tomlein <matus.tomlein@gmail.com> Co-authored-by: Deyan Deyanov <deyan@insurify.com>

Snowplow CLA bot (snowplowcla) added the cla:no label Dec 9, 2021

Matus Tomlein (matus-tomlein) requested a review from Paul Boocock (paulboocock) December 9, 2021 19:49

Matus Tomlein (matus-tomlein) force-pushed the issue/254-python_310 branch from f5fd42f to abd9ef1 Compare December 9, 2021 20:09

Paul Boocock (paulboocock) requested a review from Ada Tzereme (adatzer) December 9, 2021 21:12

Snowplow (snowplow) deleted a comment from Snowplow CLA bot (snowplowcla) Dec 9, 2021

Paul Boocock (paulboocock) removed the cla:no label Dec 9, 2021

Matus Tomlein (matus-tomlein) added 2 commits December 10, 2021 07:41

Add Dockerfile for running tests under all supported Python versions

45eba92

Matus Tomlein (matus-tomlein) force-pushed the issue/254-python_310 branch from abd9ef1 to 45eba92 Compare December 10, 2021 06:42

Paul Boocock (paulboocock) reviewed Dec 13, 2021

View reviewed changes

Ada Tzereme (adatzer) reviewed Dec 14, 2021

View reviewed changes

Matus Tomlein (matus-tomlein) added 3 commits December 15, 2021 13:38

Use Python 3.10.1 in run-tests script

879e262

Move all types aliases to typing.py and add tests for contracts

ac442e5

Add Python 3.10 to build tests on Github actions

02dbee3

Matus Tomlein (matus-tomlein) changed the base branch from master to release/0.10.0 December 15, 2021 13:13

Ada Tzereme (adatzer) approved these changes Dec 15, 2021

View reviewed changes

Matus Tomlein (matus-tomlein) merged commit 7cc11db into release/0.10.0 Dec 15, 2021

Uh oh!

Conversation

Matus Tomlein (matus-tomlein) commented Dec 9, 2021

Uh oh!

Coveralls (coveralls) commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Paul Boocock (paulboocock) commented Dec 9, 2021

Uh oh!

Paul Boocock (paulboocock) left a comment

Choose a reason for hiding this comment

Uh oh!

Ada Tzereme (adatzer) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matus Tomlein (matus-tomlein) commented Dec 15, 2021

Uh oh!

Ada Tzereme (adatzer) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Coveralls (coveralls) commented Dec 9, 2021 •

edited

Loading