Skip to content

Hatchbuild #377

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

Closed
wants to merge 0 commits into from
Closed

Conversation

fleetingbytes
Copy link
Contributor

@fleetingbytes fleetingbytes commented Nov 26, 2022

Proposal to fix #373 (Use hatchling build backend)

I have moved everything except for flake8 settings and pycodestyle settings from setup.cfg to pyproject.toml

I have prepared these scripts:

  • style:fmt runs Black and isort (all files now have imports sorted by isort)
  • style:lint runs flake8 and pycodestyle
  • docs:build runs pdoc3 to build API documentation in the api-docs directory (added to .gitignore
  • docs:serve runs pdoc3 local server on localhost:8080 to observe API docs updated live as you update docstrings. However, this somehow does not work in this project and I don't know exactly how to solve the error. Maybe @tomschr can figure it out, the API documentation is rather nice
  • test:cov runs pytest-cov
  • test:no-cov runs pytest with --no-cov
  • test:tox_test runs tox

Scripts can be run with hatch run, e.g. hatch.run style:fmt.

All the test scripts fail because of python-semver\tests\coerce.py not being a proper Python file (invalid syntax). I don't know what the purpose of this file is, so I did not touch it. It makes tests fail even if you run then natively with tox, so repairing this is beyond the scope of this PR, methinks.

I changed the classifier Environment :: Web Environment to Environment :: Console as I think it's more fitting.

Copy link
Member

@tomschr tomschr left a comment

Choose a reason for hiding this comment

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

Thanks @Nagidal for your work, very much appreciated! 👍

I've looked through the changes and left some comments below. Unfortunately, I saw this PR a bit too late, so I merged PR #375 already. I'm sorry! That might create conflicts in your branch. You need to rebase and resolve the conflicts. If you need help, let me know or I can do it for you.

Regarding the tests/coerce.py: That should be a link to docs/advanced/coerce.py. It is used in some doc tests. The link is currently broken, I see it has some strange characters at the end of the filename. If you correct these the tests should work again.

The overall change looks pretty good, I have only these two main concerns:

  • With the current pyproject.toml you can't build the user documentation. We need that. 🙂 That issue should be fixed and I've addressed it below.

  • The source codes have been changed. I would have expected that such a change wouldn't touch the source code at all. I think, this should also be fixed. This PR should NOT contain any changes to the Python code.

I would also like to see in this PR is the (user) documentation. The file BUILDING.rst should also be adapted.

Further ideas I would like to know, but we can do that in a later PR:

  • Change the Towncrier changelog. We need another entry in pyproject.toml where we can build, check, and create the changelog file.
  • Upload of wheel and source dist to PyPI. (This seems to be already integrated in hatch)
  • Create test matrices to replace tox. (Already in this PR, but the list needs to be checked again.)
  • Adapt the GitHub Actions to use hatch instead of tox could be discussed.

@@ -0,0 +1 @@
Changed build backend to hatchling
Copy link
Member

Choose a reason for hiding this comment

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

We use Git log message style: all verbs are in simple present tense.
Furthermore, I would list some other parts that comes with changing the build backend.

Suggested change
Changed build backend to hatchling
Change build backend to hatchling.
* Replace ``"Environment :: Web Environment"`` with ``"Environment :: Console"`` as it is a better fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I guess I should leave the classifier change out of the scope of this PR. We would probably make a separate issue for if and update the classifiers in a dedicated PR.

pyproject.toml Outdated
Comment on lines 101 to 108
[tool.hatch.envs.docs]
dependencies = [
"pdoc3"
]

[tool.hatch.envs.docs.scripts]
build = "pdoc --html --output-dir api-docs src/semver --force"
serve = "pdoc --http : python-semver"
Copy link
Member

Choose a reason for hiding this comment

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

This entry builds the API documentation, not the real documentation. With this current pyproject.toml nobody can build it. That is not good.

Apart from this, we have already an API documentation. It's integrated in the autogenerated Sphinx build. Why another API? Apart from the optical change, I don't see a need for pdoc at the moment. We should keep this to a minimum.

Sooo... we have here two options:

  1. Completely replace pdoc with sphinx (that would be my favorite)
  2. Rename the .docs to .apidoc as the name doesn't advertise the correct part. The .docs should be reserved only to the user documentation, not API docs.

If I continue with (1), I've added the requirements from the docs/requirements.txt file and changed the serve subcommand. This would end up with this:

Suggested change
[tool.hatch.envs.docs]
dependencies = [
"pdoc3"
]
[tool.hatch.envs.docs.scripts]
build = "pdoc --html --output-dir api-docs src/semver --force"
serve = "pdoc --http : python-semver"
[tool.hatch.envs.docs]
dependencies = [
"sphinx",
"sphinx-argparse",
"sphinx-autodoc-typehints",
]
[tool.hatch.envs.docs.scripts]
build = "make -C docs html"
linkcheck = "make -C docs linkcheck"
serve = "python3 -m webbrowser -t docs/_build/html/index.html"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot and overlooked that python-semver's documentation is made by sphinx. I will get rid of pdoc3 and bring in the sphinx API doc.

Copy link
Member

Choose a reason for hiding this comment

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

All good, absolutely no problem. 🫂 That why we have a review. 🙂

pyproject.toml Outdated
Comment on lines 133 to 137
exclude_lines = [
"no cov",
"if __name__ == .__main__.:",
"if TYPE_CHECKING:",
]
Copy link
Member

@tomschr tomschr Nov 26, 2022

Choose a reason for hiding this comment

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

  • I would prefer to change no cov to pragma: no cover as it is already used in the code.
  • Take cover the precision option from the .coveragerc file
  • Add additional lines which should be excluded (partly from .coveragerc).
  • Delete the file .coveragerc as it is not needed anymore.
Suggested change
exclude_lines = [
"no cov",
"if __name__ == .__main__.:",
"if TYPE_CHECKING:",
]
show_missing = true
precision = 1
exclude_lines = [
"pragma: no cover",
"if __name__ == .__main__.:",
"if TYPE_CHECKING:",
# note the use of single quote below to denote "raw" strings in TOML
'if not hasattr\(__builtins__, .cmp.\):',
'class .*\bProtocol\):',
'@(abc\.)?abstractmethod',
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I did not check what python-semver's code actually uses to mark excluded lines. The "no cov" setting was copy-pasted from one of my pyproject.tomls. We shall of course have here the same comments which python-semver uses.

Copy link
Member

Choose a reason for hiding this comment

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

Hehe, no problem. I assumed it was copied from somewhere. You couldn't know these details, so no worries.

@@ -0,0 +1,3 @@
Correct Towncrier's config entries in the :file:`pyproject.toml` file.
Copy link
Member

Choose a reason for hiding this comment

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

This file was integrated as part of PR #375 and should not appear in this branch. You should remove this file and rebase your branch. Possibily you need to resolve conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Will do.

pyproject.toml Outdated
@@ -40,42 +190,23 @@ template = "changelog.d/_template.rst"
# issue_format = "`#{issue} <https://github.com/python-attrs/attrs/issues/{issue}>`_"
# issue_format = ":gh:`{issue}`"

# [[tool.towncrier.type]]
Copy link
Member

Choose a reason for hiding this comment

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

The parts are already done by PR #375. The Towncrier parts below will possibly create a conflict when you rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, I shall exclude the #375 changes from this PR. Until now I was only doing PRs for my one-man show projects, never interfering with anyone else's changes upstream, So I picked up these nasty habits of mixing in other unrelated changes into the PR. I'll correct this. Happy to work with you on a more complex project and learn to produce some more disciplined PRs.

@@ -4,36 +4,12 @@
A Python module for semantic versioning. Simplifies comparing versions.
"""

from ._deprecated import (
Copy link
Member

Choose a reason for hiding this comment

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

Why is the source code changed?

I'm not entirely sure why the change of pyproject.toml triggers a reformatting. It shouldn't. The source code is perfectly formatted and the black config wasn't changed. This PR is all about switching from setuptools to hatch(ling) and we should keep it as such.

Could you please check and revert these changes in the source code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally ran the style:fmt script, which runs isort, and it reformatted (and sorted) the import statements. Using isort would be another suggestion, but again, out of scope of this PR. I'll revert these.

Copy link
Member

Choose a reason for hiding this comment

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

Again, no problem.
Actually, I was thinking about isort too. But as you said, we can do that in another PR. 👍

Copy link
Member

@tomschr tomschr left a comment

Choose a reason for hiding this comment

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

I found some other minor issues and left some questions.

pyproject.toml Outdated
]
maintainers = [
{ name = "Sebastien Celles", email = "s.celles@gmail.com" },
{ name = "Tom Schraitle" },
Copy link
Member

Choose a reason for hiding this comment

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

Does it not require an email address? You could use this:

Suggested change
{ name = "Tom Schraitle" },
{ name = "Tom Schraitle", email="tomschr@users.noreply.github.com" },

Copy link
Contributor Author

@fleetingbytes fleetingbytes Nov 26, 2022

Choose a reason for hiding this comment

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

That's not necessary. According to PEP 621:

These fields accept an array of tables with 2 keys: name and email. [...] Both keys are optional.

pyproject.toml Outdated
tox_test = "tox"

[[tool.hatch.envs.test.matrix]]
python = ["310", "311"]
Copy link
Member

@tomschr tomschr Nov 26, 2022

Choose a reason for hiding this comment

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

Why are Python 3.7, 3.8, 3.9 not in the list? This project supports 3.6/3.7 and later. Although we have a poll open to get rid of Python 3.6 (see #371).

Suggested change
python = ["310", "311"]
python = ["37", "38", "39", "310", "311",]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I would sketch up just a little test matrix in pyproject.toml for the time being, to see how it works for you and your Github workflows. All tests and their environments neatly set up in tox and it is not necessary to migrate to a hatch test matrix right away. However, we can do this. I shall update the PR with a [3.7, ... 3.11] matrix.

pyproject.toml Outdated
[tool.mypy]
# the mypy settings go here
# To have the `py.typed` file installed with the package we had to include it
# in the build metadata (see [tool.hatch.build])
Copy link
Member

Choose a reason for hiding this comment

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

I've tested it with these options:

Suggested change
# in the build metadata (see [tool.hatch.build])
# in the build metadata (see [tool.hatch.build])
enable_error_code = [
"ignore-without-code",
]
show_error_codes = true
warn_unused_ignores = true
implicit_reexport = true
pretty = true
follow_imports = "normal"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've never used mypy, and I did not look thoroughly enough where your current mypy options were until now. But of course, we shall have these here now.

pyproject.toml Outdated
# in the build metadata (see [tool.hatch.build])

[tool.pytest.ini_options]
norecursedirs = ".git build .env/ env/ .pyenv/ .tmp/ .eggs/ venv/"
Copy link
Member

Choose a reason for hiding this comment

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

I think, the norecursedirs should be a list instead of a string?

Suggested change
norecursedirs = ".git build .env/ env/ .pyenv/ .tmp/ .eggs/ venv/"
norecursedirs = [
".git",
"build",
".env/",
"env/",
".pyenv/",
".tmp/",
".eggs/",
"venv/",
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't tell from pytest's documentation whether norecursedirs accepts a list, or if it all must be one string (the same is not sure for addopts). I shall test if these config options understand lists and see.

pyproject.toml Outdated
Comment on lines 124 to 130
[tool.coverage.run]
branch = true
parallel = true
omit = [
# add files to exclude them from the coverage report, e.g.
# "src/semver/__about__.py"
]
Copy link
Member

@tomschr tomschr Nov 26, 2022

Choose a reason for hiding this comment

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

If I'm not mistaken, you need to tell coverage/pytest-cov where to find the source by using source. Furthermore, I would add semver/_types.py to the list of files to be excluded from coverage (it contains just type definitions):

Suggested change
[tool.coverage.run]
branch = true
parallel = true
omit = [
# add files to exclude them from the coverage report, e.g.
# "src/semver/__about__.py"
]
[tool.coverage.run]
source = [ "src/semver/*", ]
branch = true
parallel = true
omit = [
# add files to exclude them from the coverage report, e.g.
"src/semver/_types.py",
]

Furthermore, if you add config options to pyproject.toml you need to delete the file .coveragerc. That file isn't need anymore and we shouldn't have duplicate content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do.

pyproject.toml Outdated
Comment on lines 117 to 119
cov = "pytest -vx"
no-cov = "cov --no-cov"
tox_test = "tox"
Copy link
Member

@tomschr tomschr Nov 26, 2022

Choose a reason for hiding this comment

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

I suspect something is missing here. When I run tox (or pytest), I can run it either with tests against all defined Python versions or a specific version. Running tox alone would always run all Python versions.

That takes some time, although not much. Nevertheless, it can slow down your development, especially if you want to get results fast. For example, I usually pick one version only and run my tests against that. After I've finished my code, I run it against all (or leave that to the GitHub Actions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

umm, sure. I think it would be convenient to use the test environment for testing only with Python 3.7, and have another test environment, e.g. testall with the complete matrix of Python versions.

For running tox with all its regular tests I will prepare a hatch run tox:test script.

@fleetingbytes
Copy link
Contributor Author

I will try to correct this tomorrow, or by Monday latest. Thanks for all the suggestions and push-backs.

@tomschr
Copy link
Member

tomschr commented Nov 26, 2022

Thank you very much. You're welcome. Take your time, no need to hurry. 🙂

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.

Use hatchling build backend
2 participants