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

Add support for python3.11, drop support for python 3.7 #475

Closed
wants to merge 1 commit into from

Conversation

rasaffie
Copy link

@rasaffie rasaffie commented Sep 22, 2023

Fix issues #471 and #447

@@ -31,7 +31,7 @@
"maxLength": 20
},
"modem": {
"$ref": "#definitions/ModemType"
"$ref": "#/definitions/ModemType"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change required do to jsonschema-specifications dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this change is required? What problem does this change solves?

Copy link
Author

@rasaffie rasaffie Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong that the cause was jsonschema-specifications, I think it is another incompatibility with newer versions of jsonschema.

When removing support for Python 3.7, some libraries versions are bumped:
Screenshot 2023-09-26 at 12 12 38

Fixing the ImportError and running tests, 3 fails with error jsonschema.exceptions._WrappedReferencingError: InvalidAnchor: '#definitions/ChargingStationType' is not a valid anchor, neither as a plain name anchor nor as a JSON Pointer. You may have intended to use '#/definitions/ChargingStationType', as the slash is required *before each segment* of a JSON pointer.
Screenshot 2023-09-26 at 12 10 14

So this change only apply the suggestion from the error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting find, thanks for sharing.

When removing support for Python 3.7, some libraries versions are bumped.

That doesn't have to happen. After changing the pyproject.toml you can run poetry lock --no-update to avoid updating any packages

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong about poetry lock --no-update. Please ignore that comment.

I'm able to reproduce the issue shown in the screenshot using Python 3.10.

This assumes a clean clone from current master branch.

  1. Modify pyproject.toml to update to set minimal python version to 3.8.
diff --git a/pyproject.toml b/pyproject.toml
index f48fbcd..052272a 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -30,7 +30,7 @@ classifiers = [
 ]

 [tool.poetry.dependencies]
-python = "^3.7"
+python = "^3.8"
 jsonschema = "^4.4.0"

 [tool.poetry.dev-dependencies]
  1. Run poetry update
  2. Run poetry run pytest

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue to track this specific issue: #480.

I'm not sure what the best way to fix is to fix this error. We could modify all schemas. However, this project contains a lot of schemas. It's easy to forget to an incorrect reference.

Do you know if it's possible to use jsonschema in a way it ignores these invalid references?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't 😞

tests/test_messages.py Outdated Show resolved Hide resolved
tests/test_messages.py Outdated Show resolved Hide resolved
@@ -375,83 +393,115 @@ i18n = ["Babel (>=2.7)"]

[[package]]
name = "jsonschema"
version = "4.17.3"
Copy link
Author

@rasaffie rasaffie Sep 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev dependency asynctest for [email protected] was setting version 4.17.3 of this library, so no ImportError is triggered when running tests or in development environment.
However, when installing ocpp in a project, asynctest is not installed, so jsonschema is set to 4.19.1, causing the ImportError in production environment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the relation between asynctest and jsonschema. Can you elaborate?
When dropping support for Python 3.7, the dependency asynctest becomes obsolete and can be dropped.

Copy link
Author

@rasaffie rasaffie Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was wrong that the cause was asynctest and I didn't elaborate enough.

In [email protected] they dropped support for Python 3.7, so any project using 3.7 will receive v4.17.3 when installing the library.

In development/test environment of ocpp library, the poetry.lock file requires dependencies to support Python 3.7 (so jsonschema is fixed to v4.17.3). But in production environment, if any version of Python > 3.7 is being used, then jsonschema will bump to the latest version, that is v4.19.1 with the renamed variable, generating the ImportError.

Bottom line: if we want ocpp to be tested with the latest version of jsonschema, we need to drop support for Python 3.7.

@OrangeTux
Copy link
Collaborator

Thanks for your PR. It contains some useful changes.

I've some feedback though:

  1. Please provide one PR per issue. This PR fixes multiple issues (e.g. dropping support for Python 3.7, adding support for Python 3.11, fixing an ImportErrorfor modern version of jsonschema etc). It's good practice to isolate 1 PR per issue. Among other things, that allows it revert a single fix easily, without reverting other fixes as well.

  2. This PR introduces the dependency StrEnum to fix Python 3.11 changes behavior how enums are formatted as string #447 . I'm conservative when adding dependencies to this library. A dependency always carries the risk of causing problems. For example, it's API could change, it's license could change and so on. In Python 3.11 changes behavior how enums are formatted as string #447 I already proposed 2 fixes that don't rely on an extra dependency. I'm having a chat with my colleagues to discuss which solution mention in Python 3.11 changes behavior how enums are formatted as string #447 we prefer.

@@ -52,4 +51,4 @@ profile = "black"

[build-system]
requires = ["poetry>=1.1.11"]
build-backend = "poetry.masonry.api"
build-backend = "poetry.core.masonry.api"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change?

Copy link
Author

@rasaffie rasaffie Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be up to date with the latest documentation of Poetry, but I think both notations have the same result.

@JoeriHermans
Copy link

JoeriHermans commented Oct 7, 2023

Hi @rasaffie nice PR!

Edit: Nevermind, you already did.

I believe you also have to modify the GitHub workflow, in order to be consistent. Changing

       version:
        - "3.7"
        - "3.8"
        - "3.9"
        - "3.10"

in .github/workflows/pull-request.yml to

      version:
       - "3.8"
       - "3.9"
       - "3.10"
       - "3.11"

should do the trick I believe. Maybe it is a good idea to immediately check for Python 3.12, since it has been released? The changes should be minimal, I believe the only problem is a linting issue in ocpp/exceptions.py. In addition, there is also a small typo in the help description of the Makefile. It should be tests instead of test.

@Jared-Newell-Mobility
Copy link
Contributor

Replaced by PR:

#564
#565

So will close for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants