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

Replaced class CppDict with new class SDict, updated with latest changes from python_project_template #30

Merged

Conversation

ClaasRostock
Copy link
Collaborator

@ClaasRostock ClaasRostock commented Oct 20, 2024

Breaking change

  • class CppDict in module dictIO.cppDict has been replaced with the class SDict[_KT, _VT] in module dictIO.dict.
    In order to maintain backward compatibility, a thin wrapper class named CppDict is kept in version ~0.4.0.
    It is marked as deprecated, though, and will be removed with release 0.5.0.
  • Where CppDict inherited from UserDict, SDict inherits directly from Python's dict class,
    allowing to use it in any context where a dict or any other MutableMapping is expected.
  • SDict is generic. Static type checkers will hence require type arguments when SDict is used in type hints.
    Where you could write my_dict: CppDict = CppDict(), you will now need to specify the type arguments, e.g.
    my_dict: SDict[str, Any] = SDict(). With this change, type hinting is in line with how type hinting of Python's builtin dict works, and offers more control in static type checking.

Changed

  • Changed from pip/tox to uv as package manager
  • README.md : Completely rewrote section "Development Setup", introducing uv as package manager.
  • Changed publishing workflow to use OpenID Connect (Trusted Publisher Management) when publishing to PyPI
  • Updated copyright statement
  • VS Code settings: Turned off automatic venv activation
  • Replaced black formatter with ruff formatter

Added

  • Added instance methods dump() and load() to SDict
  • Added mypy as static type checker (in addition to pyright)

GitHub workflows

  • (all workflows): Adapted to use uv as package manager
  • _test_future.yml : updated Python version to 3.13.0-alpha - 3.13.0
  • _test_future.yml : updated name of test job to 'test313'

Dependencies

  • Updated to ruff>=0.6.3 (from ruff==0.4.2)
  • Updated to pyright>=1.1.378 (from pyright==1.1.360)
  • Updated to sourcery>=1.22 (from sourcery==1.16)
  • Updated to pytest>=8.3 (from pytest>=8.2)
  • Updated to Sphinx>=8.0 (from Sphinx>=7.3)
  • Updated to sphinx-argparse-cli>=1.17 (from sphinx-argparse-cli>=1.16)
  • Updated to myst-parser>=4.0 (from myst-parser>=3.0)
  • Updated to furo>=2024.8 (from furo>=2024.5)
  • updated to setup-python@v5 (from setup-python@v4)
  • updated to actions-gh-pages@v4 (from actions-gh-pages@v3)
  • updated to upload-artifact@v4 (from upload-artifact@v3)
  • Updated to download-artifact@v4 (from download-artifact@v3)
  • updated to checkout@v4 (from checkout@v3)

…ject_template

(files in root folder, .github, .vscode, as well as selected files in docs and tests)
…ned out not to be a solid solution. (This basically reverts commit 081992)
…ocumenting what the differences between these two methods are.
…`__ror__()`) and inner-or (`__ior__()`) special methods.
…__ror__()`) and inner-or (`__ior__()`) special methods to update also attributes.
…tance, i.e. the instance of the argument passed in, not a copy.
@frl000
Copy link
Member

frl000 commented Oct 29, 2024

I have some comments | questions for better understanding the whole thing.
First: I tend to discuss abandoning the backward compatibility not to make it too complicated.
Second: Is there a way to rename _KT and _VT to a more readable _key_type etc?
And is serializable dict a good name? And what was to origin?

@frl000
Copy link
Member

frl000 commented Oct 29, 2024

Another comment; in types.py is the type TKye and TValue to type Any. Isn't it simply taking Any as type and simply doing nothing?

@ClaasRostock
Copy link
Collaborator Author

Hi Frank, @frl000

good comments and questions.

Probably best to have a chat :-) However, I try to give a first response in written, following the three aspects you raised:

(1) Backward compatibility
I think it is a good idea to keep backward compatibility at least for a grace period; and it is comparatively cheap. It is really only the thin CppDict wrapper class plus the data property in SDict that are needed for backward compatibilty. Given that also we ourselves need this backward compatibility for a transition period until we adapted our own code in dictIO, farn and mvf to use the new SDict class, I feel it is worth the effort. We can introduce the CppDict wrapper class now in v0.4.0, and then finally remove it with v0.5.0. Let me know what you think.

(2) Naming of generic type variables for the key and value type in SDict
Good and valid comment. I was neither 100 % happy with _KT and _VT. The reason I used these is because we used them before in CppDict. Back then, when I created CppDict, I think these names had simply be used in an example I found..
Anyway - good reason to investigate whether there is a common naming convention for generic type variables!
So I asked Copilot:

Prompt:
In Python, I created a class SDict which inerits from Python's builtin dict class. The dict class has two generic type variables, one for for the key type, and one for value type. In my class SDict, I want to mirror this and define the class with two generic type variables, one for SDict's key type, and one for SDict's value type. My question is: Is there a naming convention for the generic type variables of Python's dict class ?

Response:
In Python, the naming convention for generic type variables in the dict class typically follows the pattern of using single uppercase letters. For example, the dict class uses K for the key type and V for the value type. This convention is also commonly used in other generic classes and functions in Python.
Here's an example of how you can define your SDict class with two generic type variables:

from typing import TypeVar, Generic

K = TypeVar('K')
V = TypeVar('V')

class SDict(dict[K, V], Generic[K, V]):
    pass

In this example, K and V are used as the generic type variables for the key and value types, respectively. This follows the common convention and makes your code more readable and consistent with other generic classes in Python[1][2].
References
[1] PEP 585 – Type Hinting Generics In Standard Collections
[2] PEP 695 – Type Parameter Syntax | peps.python.org

So, based on that, how about changing _KT to K, and _VT to V ?

(3) Type aliases TKey and TValue set to Any
This is true. The reason I introduced the indirection via type aliases TKey and TValue instead of using just Any wherever key and / or value type needs to be declared, is that I wanted to have one place where the permissible types for keys and values can centrally be managed and changed. Very obviously, as you correctly observed :-) , I did not play that card yet.

But, if we are honest, we currently would need to limit key type, for instance, to str. Just because, given the current implementation in dictIO, we cannot serialize dicts that have key types other than str.
So then, having TKey and Value as type aliases, we can apply such a change in permissible types in one central location, and immediately see whether static typing in the code base still resolves.

Well, that was the idea. When I did so for a quick test, though, mypy and pyright immmediatly raised so many issues that I turned types back to Any - for now. But having the type aliases Tkey and TValue allows us to take a second dive into that topic, and improve type annotations to have dictIO properly communicate that, for instance, keys need to be of str type currently. It is just that I wanted to push forward that into a separate PR, in order not to overstress what we change with this PR (it is already a lot), and also in order to get v0.4.0 shipped so that we can start adjustments in ospx, farn and mvf to use the new SDict class in these packages.

…bers was generated twice and added extension to support Markdown-based diagrams created with Mermaid.
…hon_project_template

# Conflicts:
#	src/dictIO/cli/dictParser.py
#	src/dictIO/cppDict.py
#	src/dictIO/dictParser.py
#	src/dictIO/dictReader.py
#	src/dictIO/formatter.py
#	src/dictIO/parser.py
#	src/dictIO/utils/counter.py
#	src/dictIO/utils/strings.py
#	tests/test_dictReader.py
#	tests/test_dictWriter.py
#	tests/test_formatter.py
#	tests/test_parser.py
#	tests/utils/test_path.py
…(non-reusable) workflow. Remove the two reusable workflows _publish_package.yml and _publish_package_test.yml.

Background for this change is a limitation recently introduced on pypi, which does not allow anylonger to run the GitHub action 'pypa/gh-action-pypi-publish' from a reusable workflow. The code hence needed to be moved upwards, from the reusable workflow _publish_package.yml into the (non-reusable) workflow publish_release.yml
See https://github.com/marketplace/actions/pypi-publish -> Note under "Trusted Publishing"
@ClaasRostock ClaasRostock merged commit f638129 into main Nov 10, 2024
13 checks passed
@ClaasRostock ClaasRostock deleted the update_with_latest_changes_from_python_project_template branch November 10, 2024 20:25
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