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

Modernize project #188

Merged
merged 20 commits into from
May 8, 2024
Merged

Modernize project #188

merged 20 commits into from
May 8, 2024

Conversation

bbannier
Copy link
Member

This PR contains a number of cleanups of the project.

  • Move most project config from setup.py to pyproject.toml: with this one can use modern tools like e.g., rye or hatch to manage dev setups for this project
  • Move formatting/linting to ruff/ruff-format: we have already done this for a number of other projects
  • Activate additional lints. These seemed overall non-intrusive to me and with low rate of false positives (ruff could also fix most of them automatically). This list could be expanded, but I stopped here.
  • Misc. cleanups

@bbannier bbannier self-assigned this Apr 13, 2024
@bbannier bbannier marked this pull request as ready for review April 13, 2024 20:16
@bbannier bbannier requested a review from ckreibich April 13, 2024 20:16
@bbannier bbannier mentioned this pull request Apr 13, 2024
@bbannier bbannier force-pushed the topic/bbannier/modernize branch from 2f9be6a to 66dfbf3 Compare April 15, 2024 07:57
bbannier added 20 commits April 18, 2024 21:47
We previously used a mix of `setup.py` and `setup.cfg` which are not
well supported by more modern Python developer tools which instead
prefer `pyproject.toml`. This meant that developing this project
required manually setting up and managing virtualenvs.

With this patch we now move most of our configuration over into
`pyproject.toml` so especially dev environments can be managed with
tools like Hatch[^1] or Rye[^2]. Dev envs managed by these tools are
automaticallt discovered by many editors so there is potentially less
need to even think about virtualenvs.

Now `setup.py` contains the absolute minimum required settings. We still
need to keep dynamically generating the package version from the
Zeek-style `VERSION` file, and also need to add a tweak to make sure it
is distributed in wheels so the installation code reading it has access
to it.

Since settings in `setup.cfg` can conflict with `pyproject.toml` we
delete the file after either moving all setttings to `pyproject.toml` or
to tool specific files (flake8 still does not support `pyproject.toml`).

For now we keep `requirements.txt` (which updated versions) so we can
install dev dependencies, e.g., for RTD. Unfortunately dev dependencies
in `pyproject.toml` are still not standardized[^3].

[^1]: hatch.pypa.io
[^2]: https://rye-up.com/
[^3]: https://discuss.python.org/t/development-dependencies-in-pyproject-toml/26149
@bbannier bbannier force-pushed the topic/bbannier/modernize branch from 66dfbf3 to c95a823 Compare April 18, 2024 19:48
Copy link
Member

@timwoj timwoj left a comment

Choose a reason for hiding this comment

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

I looked over all of it but most of it looks like automated changes. I didn't see anything out of place. The change to pyproject.toml looks good to me too.


[project.optional-dependencies]
dev = [
"btest>=1.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

The change to move btest from the default dependencies into a dev dependency should be the only potentially controversial change here. This should not be an issue for users who get their zkg via Zeek since it also ships btest; users who install zkg via e.g., pipx will not get btest since when installing zkg pipx would only create a link for zkg, not for scripts from dependencies. This leaves users who install directly (e.g., via pip) as the only ones potentially affected (but I would argue they by using such a low-level approach they already decided that they'd want maximum control).

I discussed this out-of-band with @timwoj and he agreed that dropping btest as a dep didn't seem unreasonable to him. The only potential issue could be that we currently do not publish release notes for zkg so it is unclear where to call this out (if at all).

@timwoj timwoj merged commit 0bf46b0 into zeek:master May 8, 2024
5 checks passed
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