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

various amendments #21

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

various amendments #21

wants to merge 25 commits into from

Conversation

dornech
Copy link

@dornech dornech commented Dec 22, 2024

Hi @FlorianWilhelm,
I spent some time on your Hatchlor.
I did some amendments you left over: I included two options for local comitting and versioning - bump-my-version/generate-changelog or semantic-release. Both are - if set up - easy to use. I included a basic config for both tools in the pyproject.toml template so everbody is free to use the one or the other.
Furthermore I added gitlint which I find useful to enforce conventional commit compliant commit messages - pretty useful if you automate the changelog generation with generate_changelog or semantic-release. gitlint is cool stuff and for example works also within PyCharm which is my favorite IDE (which is probably not surprising for the more professional Python developer than I would claim for myself).
Finally I did some rework on the GitHub workflows.
These are the main changes I think. Please find approriate comments in the README.md as well.

Copy link
Owner

@FlorianWilhelm FlorianWilhelm left a comment

Choose a reason for hiding this comment

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

Thanks @dornech! I added a few comments.

Would you be interested in becoming a co-maintainer for The Hatchlor? 🌹

# own developed alternative variant to hatch-vcs-footgun overcoming problem of ignored setuptools_scm settings
# from hatch-based pyproject.toml libraries
from hatch.cli import hatch
from click.testing import CliRunner
Copy link
Owner

Choose a reason for hiding this comment

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

What is the advantage of this compared to the original version? It expects way more packages to be installed during execution time like click and hatch itself, which normally is not required to be present when you just want to install the package you have developed.

Copy link
Author

@dornech dornech Dec 23, 2024

Choose a reason for hiding this comment

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

This is required for retrieving the actual build version. It is an adjusted version of the footgun as commented.
I agree this might be overhead for non-developer installation but if hatch is installed anyway, there is no additional overhead.

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure I really get it. My original code accomplished the same thing. It read out the version but only using modules that are shipped directly with Python. So the big advantage of my original code is that you don't need to have hatch installed to use the package. I know that developers of a package need to have hatch installed but you should always think about the users of what you are building. They should not be required to install hatch.

Copy link
Author

Choose a reason for hiding this comment

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

If you do an "pip install -e" then you always see the last install version -- not the last commit version.
It is explained here:
https://github.com/maresb/hatch-vcs-footgun-example

But I agree: this is probably a small advantage compared to the setuptool_scm dependency. And it is only neceesary if one has very strict version dependencies in the code.
Probably refering to metadata or the local _version.py after a hatch build is a good compomise?

Copy link
Author

@dornech dornech Dec 25, 2024

Choose a reason for hiding this comment

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

Argh, I did miss that I eliminated the setuptools_scm dependency already because of using Hatch compared to the original footgun-version. the setuptools_scm did not work correctly with the version bumping.
Explained in more detail here - maresb/hatch-vcs-footgun-example#3: @maresb made also an suggestion how to call hatchling directly (without the click-overhead of Hatch as the front-end to hatchling).

@@ -71,10 +77,104 @@ version-file = "src/{{ cookiecutter.pkg_name }}/_version.py"
packages = ["src/{{ cookiecutter.pkg_name }}"]

[tool.hatch.build.targets.sdist]
artifacts = ["_version.py"]
Copy link
Owner

Choose a reason for hiding this comment

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

Smart, so this is necessary for source distributions so that it is included?

Copy link
Author

Choose a reason for hiding this comment

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

The _verison.py file is generated during build and is to be included for the installable installation I think.

Copy link
Owner

Choose a reason for hiding this comment

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

This worked out of the box for wheel files but not sure about sdists. Good point.

Copy link
Author

Choose a reason for hiding this comment

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

To be honest I am not experienced with the difference between wheel and sdist-distributions.

"Programming Language :: Python :: Implementation :: CPython",
]
# direct dependencies of this package, installed when users `pip install {{ cookiecutter.project_slug }}` later.
dependencies = [ # ToDo: Modify according to your needs!
"typer",
"setuptools_scm",
Copy link
Owner

Choose a reason for hiding this comment

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

setuptools_scm should not be needed for a normal, i.e. non-development, installation. It's not necessary to have it as a runtime requirement.

Copy link
Author

Choose a reason for hiding this comment

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

I agree but it is the issue as above: it is the hatch-vcs-footgun in init.py ...

Copy link
Author

Choose a reason for hiding this comment

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

Argh, I did miss that I eliminated the setuptools_scm dependency already because of using hatch compared to the original footgun-version. the setuptools_scm did not work correctly with the version bumping.

Copy link
Author

Choose a reason for hiding this comment

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

BTW - the issue with setuptools_scm was discussed here
maresb/hatch-vcs-footgun-example#3

hooks:
- id: mypy
args: ["--install-types", "--non-interactive"]
additional_dependencies: [types-tabulate, types-cachetools]

- repo: https://github.com/jorisroovers/gitlint
Copy link
Owner

Choose a reason for hiding this comment

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

I really like the idea but would it be better to have this optional via cookiecutter? Gitlint is still having less than 1k Github stars and is it really used that often? I like it but it's not a commonly found best practice, is it?

Copy link
Author

Choose a reason for hiding this comment

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

Well, to be honest: gitlint was the only commit-message linter I found. For automated generation of a changelog it is necessary to have proper commit messages. I struggled somewhat on that ...
Probably more professional developers than I am are much more familiar with this and do it while asleep but for me it was helpful.
I do not knoy how cookiecutter would be of any help here but probably I missed something there.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I guess now I know what you mean with "optional via cookiecutter". You mean similar to the lock-file support i. e. controlling via y/n prompt during cookiecutter execution? I am afraid it is me lacking some cookiecutter / Python packaging experience that I did not get it directly. Sounds like a valid option.

Probably also an option fot the complete footgun-stuff ...

Copy link
Owner

Choose a reason for hiding this comment

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

Exactly, to make it gitlint an additional option. Bear in mind though that with every additional option, running unittests becomes more complicated since you need to test every possible path, not only the one that includes all features.

Copy link
Author

Choose a reason for hiding this comment

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

Question: to do it perfect, I would have to delete the .gitlint config file after generation via the post_gen_project.py hook ?

Copy link
Author

@dornech dornech Dec 25, 2024

Choose a reason for hiding this comment

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

I tried to make the gitlint optional. Struggling with the post-generation hook to delete the .gitlint file, other error fixed.

@@ -38,4 +45,4 @@ jobs:
hatch run lint:all
- name: Run Tests
run: |
hatch run test:pytest
hatch test
Copy link
Owner

Choose a reason for hiding this comment

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

Nice, the new way of doing this! Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

My idea was to leave the testing for different versions and environments mainly with GitHub. This is achieved by setting the matrix for os / python version in the GitHub action and executing a hatch test in the standard hatch-test environment.
I was only struggling a little bit with the package dependencies, there might be some room for optimization.

@dornech
Copy link
Author

dornech commented Dec 23, 2024

General remark: I am afraid this pull request still has some escaping errors. Fix see subsequent commits from today.

@dornech
Copy link
Author

dornech commented Dec 23, 2024

Regarding co-maintainer: I feel honoured you are asking. I am just not sure if I am experienced enough. I mean I think I am not a rookie in IT and in development but at least a newbie in the Python Packaging universe ...

@dornech
Copy link
Author

dornech commented Dec 24, 2024

I would love to see PyScaffold become Hatch-enabled to allow to work with Hatch-based scaffolds and probably migrating Hatchlor to a PyScaffold template. So far I am struggling with Cruft which is less well maintained and does not work in my environment.

@FlorianWilhelm
Copy link
Owner

PyScaffold follows a completely different approach and uses traditional tools like setuptools only. To make it hatch-based would mean to rewrite almost everything... Not sure that really makes sense and that this should be done. What would you gain from it? The idea was to have a lightweight Hatchlor for people using hatch (and there aren't that many) and PyScaffold for developers that want to stick with a traditional toolchain.

@FlorianWilhelm FlorianWilhelm added the enhancement New feature or request label Dec 25, 2024
@dornech
Copy link
Author

dornech commented Dec 25, 2024

Hm. My understanding was that PyScaffold helps to follow and adopt changes in an underlying template. I wonder why this should not be of interest for Hatch based delopment as for "traditional" toolchain. Only other tool I found is cruft but it is less well maintained and I am not yet too confident in this tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants