Skip to content

Refactoring/#392 refactor pre commit hook package version.py into nox task #412

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Refactoring/#392 refactor pre commit hook package version.py into nox task #412

wants to merge 14 commits into from

Conversation

Jannis-Mittenzwei
Copy link
Contributor

@Jannis-Mittenzwei Jannis-Mittenzwei commented Apr 29, 2025 •
edited by ArBridgeman
Loading

Closes #392

@Jannis-Mittenzwei Jannis-Mittenzwei self-assigned this Apr 29, 2025
ArBridgeman
ArBridgeman requested changes Apr 29, 2025
View reviewed changes
@ArBridgeman ArBridgeman self-requested a review April 30, 2025 08:33
@Jannis-Mittenzwei Jannis-Mittenzwei force-pushed the refactoring/#392-refactor-pre-commit-hook-package_version.py-into-nox-task branch from b30e169 to 5fc4cc2 Compare April 30, 2025 10:37
ArBridgeman
ArBridgeman requested changes Apr 30, 2025
View reviewed changes
doc/changes/unreleased.md
* [#378](https://github.com/exasol/python-toolbox/pull/378/files): Add Nox task to trigger a release
Copy link
Collaborator

Choose a reason for hiding this comment

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

my bad 😞 should have caught last time. These should refer to the issue number.

Suggested change
* [#378](https://github.com/exasol/python-toolbox/pull/378/files): Add Nox task to trigger a release
* [#368](https://github.com/exasol/python-toolbox/issues/368): Add Nox task to trigger a release

doc/changes/unreleased.md
## ⚒️ Refactorings

* [#412](https://github.com/exasol/python-toolbox/pull/412): Refactor pre commit hook package version.py into nox task
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* [#412](https://github.com/exasol/python-toolbox/pull/412): Refactor pre commit hook package version.py into nox task
* [#392](https://github.com/exasol/python-toolbox/issues/392): Refactor pre-commit hook package version.py into nox task

project-template/{{cookiecutter.repo_name}}/exasol/{{cookiecutter.package_name}}/version.py
@@ -1,9 +1,9 @@
# ATTENTION:
# This file is generated by exasol/toolbox/pre_commit_hooks/package_version.py when using:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# This file is generated by exasol/toolbox/pre_commit_hooks/package_version.py when using:
# This file is generated by exasol/toolbox/nox/_package_version.py when using:

.github/workflows/checks.yml
@@ -19,7 +19,7 @@ jobs:
uses: ./.github/actions/python-environment
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a brief description to #392 answering:

  • what we want to change
  • why we want to make this change

test/unit/version_test.py
@@ -0,0 +1,207 @@
import subprocess
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move:

  • test/unit/version_test.py into test/unit/util/version_test.py.
  • mock_from_poetry & TestTriggerReleaseWithMocking back into test/unit/release_test.py. (These are used to test the _trigger_release function in release.__init__.py.)

test/unit/version_test.py
def test_version_from_python_no_module_error(tmp_path):
file_path = tmp_path / "file"
file_path.write_text("")
with pytest.raises(ToolboxError) as ex:
Copy link
Collaborator

@ArBridgeman ArBridgeman Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

currently, you aren't testing the value of ex, so it can be removed. you could check to see if (part of) a string matched the text, like assert "version within module" in str(ex.value).

There's a debate about checking error message strings. Some folks say it's useful especially when ToolboxError could be tossed at a multitude of spots to verify that it you got the error at the place you expected it & not somewhere else. Whereas, others think it can be problematic, as when you update the string text, you'll need to update the text in the test. In more complicated code, I'm a bit more in the first group, but I'll let you decide what you prefer to do here.

Suggested change
with pytest.raises(ToolboxError) as ex:
with pytest.raises(ToolboxError):

exasol/toolbox/nox/_package_version.py
@@ -1,33 +1,29 @@
import subprocess
import argparse
import sys
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the following:

  • exasol/toolbox/pre_commit_hooks directory, as no longer has code in it
  • doc/developer_guide/modules/pre_commit_hooks.rst as no longer relevant

Please update:

  • doc/developer_guide/modules/modules.rst by removing pre_commit_hooks entry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArBridgeman ArBridgeman ArBridgeman requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@Jannis-Mittenzwei Jannis-Mittenzwei

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🔧 refactor pre-commit-hook package_version.py into nox task
2 participants
@Jannis-Mittenzwei @ArBridgeman