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

feat: migrate from setup.py/setuptools to pyproject.toml/hatch #1163

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

Conversation

regisb
Copy link
Contributor

@regisb regisb commented Nov 22, 2024

While investigating issue #956, I realized that the issue might be resolved if we switched from setuptools to hatch as package builder. But to achieve that, we also need to migrate from setup.py to pyproject.toml. This was a long overdue chore, as setup.py is no longer recommended (see PEP 621).

I'm not sure whether we should be merging this change now, later (or ever). I think this should be safe... So I would lean towards merging it now (and eventually migrate plugins and cookiecutter to pyproject.toml as well). But I'm curious what ya'll think @kdmccormick @DawoudSheraz.

This is an attempt both to modernize the packaging and to fix mypy
testing in editable mode (issue #956).

Packaging seems to work, but mypy testing still fails. Will now attempt
to migrate from setuptools to hatch for building.
The official Python packaging guide recommends hatch over setuptools.
Beyond that official recommendation, getting rid of setuptools allows us
to resolve issue #956 on mypy-checking of plugins in editable mode.

Close #956.
@regisb regisb force-pushed the regisb/pyproject.toml branch from a071906 to 73441d2 Compare November 25, 2024 10:11
pip install .
pip install -r requirements/dev.txt

bootstrap-dev-plugins: bootstrap-dev ## Install dev requirements and all supported plugins
Copy link
Contributor

@DawoudSheraz DawoudSheraz Nov 25, 2024

Choose a reason for hiding this comment

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

Any particular reasons for merging this? I get this is being made part of full, just thinking if someone was using this in their automation and can break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The make are not supposed to be part of the Tutor API, so I think it's OK to make breaking changes here. The goal of this change is to get rid of an unused command.

Comment on lines +12 to +14
pip-compile ${COMPILE_OPTS} requirements/base.in
pip-compile ${COMPILE_OPTS} requirements/dev.in
pip-compile ${COMPILE_OPTS} requirements/docs.in
Copy link
Contributor

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 having COMPILE_OPTS? What values can it have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a technique that I also used in edx-platform to de-duplicate the list of *.in files in the Makefile.

Faraz32123 pushed a commit to overhangio/tutor-discovery that referenced this pull request Jan 24, 2025
- migrate from setup.py/setuptools to pyproject.toml/hatch.
- This commit will keep the tutor-discovery in sync with the tutor core. For more details view this PR in tutor: overhangio/tutor#1163.
Faraz32123 pushed a commit to overhangio/tutor-discovery that referenced this pull request Jan 24, 2025
- migrate from setup.py/setuptools to pyproject.toml/hatch.
- This commit will keep the tutor-discovery in sync with the tutor core. For more details view this PR in tutor: overhangio/tutor#1163.
Faraz32123 pushed a commit to overhangio/tutor-discovery that referenced this pull request Jan 27, 2025
- migrate from setup.py/setuptools to pyproject.toml/hatch.
- This commit will keep the tutor-discovery in sync with the tutor core. For more details view this PR in tutor: overhangio/tutor#1163.
@kdmccormick kdmccormick removed their request for review January 31, 2025 17:56

[project]
name = "tutor"
license = {file = "LICENSE.txt"}
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are now using the entire license file as the license, running pip show tutor displays the entire license file against the license field making it a cluttered mess if someone wants to read the other attributes of the package.

Instead I propose changing this to a text:

license = {text = "AGPL-3.0-only"}

The license names are available here.

CC: @DawoudSheraz

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for highlighting this. Using text should be good enough, the entire license content will certainly make pip show difficult to navigate. Also, I don't think we need to have dict with text key, we can add the license name directly

license = "AGPL-3.0-only"

Copy link
Contributor

@Danyal-Faheem Danyal-Faheem Feb 6, 2025

Choose a reason for hiding this comment

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

Also, I don't think we need to have dict with text key, we can add the license name directly

Unfortunately, it doesn't work for me without the dict and text key, it shows empty otherwise 😕.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants