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

Fix supported Python version #40

Closed
wants to merge 1 commit into from
Closed

Fix supported Python version #40

wants to merge 1 commit into from

Conversation

n1k61n
Copy link

@n1k61n n1k61n commented Nov 28, 2024

Update Python Version in pyproject.toml
Remove Unused Dependencies
Add Missing Dependency (requests)
Migrate Dev Dependencies to the New Style
Ensure everything works by testing the environment
poetry run pytest

@mcsken mcsken self-requested a review December 2, 2024 21:03
Comment on lines +4 to +5
from dotenv import load_dotenv
load_dotenv()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why did you create a new test file that doesn't align with any module and that does nothing but load the environment variables?

This was not part of the requirements.

You mentioned in the PR "Ensure everything works by testing the environment", but nothing is being actually tested here. And no environment-variable-dependent test is needed to complete the requirements of this issue.

pyyaml = "^6.0.2"
python-dotenv = "^1.0.1"
pytest-mock = "^3.14.0"
pydantic = "^2.10.2"
pyjwt = "^2.9.0"
requests-mock = "^1.12.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

requests-mock should have been removed from the main dependencies when it was moved to dev dependencies.

Comment on lines +64 to +68
[tool.poetry.group.dev.dependencies]
cryptography = "^44.0.0"
requests-mock = "^1.12.1"
python-dotenv = "^1.0.1"

Copy link
Collaborator

Choose a reason for hiding this comment

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

coverage, pytest, pytest-cov, pre-commit and ruff should have been migrated from the old-style dev dependencies group to the new one here, and the old dev dependencies section should have been as a result removed.

@@ -19,7 +19,7 @@ repository = "https://github.com/siisurit/check_done.git"
documentation = "https://github.com/siisurit/check_done/blob/main/README.md"
keywords = ["check", "closed", "done", "done done", "finished", "issue", "task", "validate"]
classifiers = [
"Development Status :: 4 - Beta",
"Development Status :: 4 - Beta",
Copy link
Collaborator

@mcsken mcsken Dec 3, 2024

Choose a reason for hiding this comment

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

The original indentation was correct, should be reverted.

Comment on lines +4 to +7
<content url="file://$MODULE_DIR$">
<excludeFolder url="file://$MODULE_DIR$/.venv" />
</content>
<orderEntry type="jdk" jdkName="Python 3.12 (check_done)" jdkType="Python SDK" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Contributors should either adhere to the project's IDE configuration by renaming the virtual environment used for the project to match the existing IDE project settings, or not commit this file. This should be reverted.

rich = ">=9, <14"
gitpython = "^3"
python = ">=3.11, <3.14"

Copy link
Collaborator

@mcsken mcsken Dec 3, 2024

Choose a reason for hiding this comment

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

Unnecessary empty space, should be removed.

@mcsken mcsken added this to the v1.1.0 milestone Dec 3, 2024
@roskakori
Copy link
Contributor

Rejecting in favor of the cleaner PR #41.

@roskakori roskakori closed this Dec 7, 2024
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.

3 participants