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

Modernization proposal #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Bloodmallet
Copy link

@Bloodmallet Bloodmallet commented Mar 5, 2023

Greetings,
this PR showcases a more strict following of a src layout and a modernization of the used tooling. This setup works for a flat-layout as well. While technically this is a PR, I'm aware and agree that too much is going on in here. But I felt creating a bunch of different PRs wouldn't serve the purpose of a bundled communication about the topic.

Motivation

The usage of a setup.py file is deprecated since setuptools>58.2.0 and setup.cfg will be too. Therefore when setting up a new project it would be more future proof to use a pyproject.toml file instead.

Changes

  • Introduction of src-layout
  • Replaced setup.py with pyproject.toml (including all dynamic parts)
  • Fix package __init__.py import to work properly in src-layout
  • Fix test import to assume an installed local version of the module
  • Add build dependency to dev optional dependencies for local building. E.g. python -m build . will build your wheel and tar.gz files. The usage of build is encouraged[1][2].
  • Add package README.md to root README.md

Breaking changes:

Running tests requires installing the package locally.
While this is an additional (potentially annoying) step,
it ensures we're testing exactly the package as it will be
delivered to users.

Warning:

Automatic discovery of modules is a BETA-feature of pyproject.toml.
https://setuptools.pypa.io/en/latest/userguide/package_discovery.html

Introduction of src-layout
Replaced `setup.py` with `pyproject.toml` (including all dynamic parts)
Fix package `__init__.py` import to work properly in src-layout
Fix test import to assume an installed local version of the module
Add `build` dependency for local building
Add package README.md to root README.md

Breaking changes:
Running tests requires installing the package locally.
While this is an additional (potentially annoying) step,
it ensures we're testing exactly the package as it will be
delivered to users.

Warning:
Automatic dicovery of modules is a BETA-feature of `pyproject.toml`.
https://setuptools.pypa.io/en/latest/userguide/package_discovery.html
@Bloodmallet
Copy link
Author

PS: Thank you for doing these videos. Your content is great.

Copy link

@NRCgg NRCgg left a comment

Choose a reason for hiding this comment

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

I wholeheartedly agree, moving this to the modern "moving forward" approach is a good idea and would serve the community well. The changes look good but I have not tested them myself.

@Bloodmallet
Copy link
Author

I just noticed, that I forgot to include instructions on how to test now. Here is handy one-liner to do the following:

  • create a virtual environment
  • activate it
  • install package into it
  • execute tests

Windows:

python -m venv env; ./env/Scripts/activate; pip install .[dev]; python -m pytest tests/

Linux:

python -m venv env && source ./env/bin/activate && pip install .[dev] && python -m pytest tests/

@egges
Copy link
Contributor

egges commented Mar 6, 2023

Thanks for the suggestions! I'll won't merge this pull request so that the code still matches the video, but I'll take a look at these ideas for future code examples. In any case I'm going to move to a toml file for my future code examples as I'm now also integrating Poetry in my own workflow.

@HenriqueAJNB
Copy link

HenriqueAJNB commented Mar 20, 2023

My only suggestion is to change code snippet formatting from bold to code in all .md files.

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.

4 participants