Update linters and adjust code #132
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR modernizes the project's development tooling by migrating from older linting tools (black, isort, flake8, pylint) to ruff and mypy, and updates the minimum supported Python version from 3.6 to 3.7. The changes include comprehensive updates to configuration files, CI/CD workflows, and code adjustments to comply with the new linters' requirements.
- Replaces multiple linting tools with ruff and mypy for improved performance and consistency
- Updates minimum Python version from 3.6 to 3.7 across all configuration files
- Adds type hints and converts relative imports to absolute imports for better code maintainability
Reviewed changes
Copilot reviewed 37 out of 41 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Replaces old linter configurations (black, isort, pylint) with ruff and mypy settings, adds coverage and pytest configuration |
| setup.py | Updates minimum Python version to 3.7 and removes redundant "r" mode from file open |
| sonar-project.properties | Removes Python 3.6 from supported versions |
| README.md | Updates Python version badge from 3.6 to 3.7 |
| .github/workflows/lint-and-test.yml | Replaces old linter jobs with ruff/mypy, adds Python 3.7-3.12 test matrix |
| tests/integration_test.py | Adds new integration test file with environment-based credentials |
| tests/test_helpers_items.py | Adds return type hint to MockedItem.init |
| amazon_paapi/api.py | Adds return type hints, converts None defaults to Optional types, fixes docstring formatting |
| amazon_paapi/helpers/*.py | Converts relative imports to absolute, adds return type hints, refactors error message creation |
| amazon_paapi/models/*.py | Converts relative imports to absolute imports |
| amazon_paapi/errors/exceptions.py | Adds return type hint and fixes docstring |
| amazon_paapi/init.py | Fixes docstring punctuation |
| amazon_paapi/sdk/NOTICE.txt | Removes trailing whitespace |
| Makefile | Adds new Makefile with Docker-based development commands |
| Dockerfile | Adds Dockerfile for development environment |
| .pre-commit-config.yaml | Adds pre-commit configuration with ruff, mypy, and test hooks |
| .githooks/pre-commit | Updates pre-commit hook to use Docker and pre-commit framework |
| .env.template | Adds template for environment variables |
| LICENSE | Updates copyright year to 2024 |
| .github/ISSUE_TEMPLATE/*.md | Removes trailing whitespace from issue templates |
| scripts/* | Removes old bash scripts for linting and testing (replaced by Makefile and pre-commit) |
| .flake8 | Removes flake8 configuration (replaced by ruff) |
| .coveragerc | Removes coverage configuration (moved to pyproject.toml) |
| .shellcheckrc | Removes shellcheck configuration |
| .githooks/pre-push | Removes old pre-push hook |
| examples/example.py | Removes example file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from amazon_paapi.api import AmazonApi | ||
|
|
||
|
|
||
| def get_api_credentials() -> tuple[str]: |
There was a problem hiding this comment.
The return type hint tuple[str] is incorrect. This function returns 4 string values (api_key, api_secret, affiliate_tag, country_code), but these can be None since os.environ.get() returns None if the environment variable is not set. The correct type hint should be tuple[str | None, str | None, str | None, str | None] or tuple[Optional[str], Optional[str], Optional[str], Optional[str]].
| ``ApiRequestException`` | ||
| ``ItemsNotFoundException`` | ||
| """ | ||
There was a problem hiding this comment.
The docstring closing """ should be placed before the blank line, not after it. According to standard Python docstring conventions (PEP 257), the closing quotes should immediately follow the content without trailing blank lines inside the docstring. The blank line should come after the closing quotes, separating the docstring from the code.
| ``ApiRequestException`` | ||
| ``ItemsNotFoundException`` | ||
| """ | ||
There was a problem hiding this comment.
The docstring closing """ should be placed before the blank line, not after it. According to standard Python docstring conventions (PEP 257), the closing quotes should immediately follow the content without trailing blank lines inside the docstring. The blank line should come after the closing quotes, separating the docstring from the code.
| ``ApiRequestException`` | ||
| ``ItemsNotFoundException`` | ||
| """ | ||
There was a problem hiding this comment.
The docstring closing """ should be placed before the blank line, not after it. According to standard Python docstring conventions (PEP 257), the closing quotes should immediately follow the content without trailing blank lines inside the docstring. The blank line should come after the closing quotes, separating the docstring from the code.
| ``ApiRequestException`` | ||
| ``ItemsNotFoundException`` | ||
| """ | ||
There was a problem hiding this comment.
The docstring closing """ should be placed before the blank line, not after it. According to standard Python docstring conventions (PEP 257), the closing quotes should immediately follow the content without trailing blank lines inside the docstring. The blank line should come after the closing quotes, separating the docstring from the code.
| touch .env | ||
|
|
||
| docker run -t --rm -u "$(id -u):$(id -g)" -v "${PWD}:/code" --env-file .env \ | ||
| python-amazon-paapi -c "python -m pre_commit" |
There was a problem hiding this comment.
The command python -m pre_commit is incorrect. The correct module name is pre_commit with an underscore, but when running as a module, it should be invoked as python -m pre_commit run to execute the hooks. The current command will likely fail with an error about running pre-commit as a module.
| python-amazon-paapi -c "python -m pre_commit" | |
| python-amazon-paapi -c "python -m pre_commit run" |
| needs: [test-py38] | ||
| container: | ||
| image: python:3.12 | ||
| image: python:3.7 | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v4 | ||
| - name: Install dependencies | ||
| run: pip install flake8 | ||
| - name: Check code errors | ||
| run: flake8 . | ||
| run: pip install -e . && pip install pytest | ||
| - name: Run tests | ||
| run: python -m pytest | ||
|
|
||
| pylint: | ||
| test-py38: | ||
| runs-on: ubuntu-latest | ||
| needs: [test-py39] | ||
| container: | ||
| image: python:3.12 | ||
| image: python:3.8 | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v4 | ||
| - name: Install dependencies | ||
| run: pip install pylint | ||
| - name: Check code errors | ||
| run: find . -type f -name '*.py' | xargs pylint --disable=missing-docstring --disable=too-few-public-methods | ||
| run: pip install -e . && pip install pytest | ||
| - name: Run tests | ||
| run: python -m pytest | ||
|
|
||
| test-py39: | ||
| runs-on: ubuntu-latest | ||
| needs: [test-py310] | ||
| container: | ||
| image: python:3.9 | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v4 | ||
| - name: Install dependencies | ||
| run: pip install -e . && pip install pytest | ||
| - name: Run tests | ||
| run: python -m pytest | ||
|
|
||
| test-py310: | ||
| runs-on: ubuntu-latest | ||
| needs: [test-py311] | ||
| container: | ||
| image: python:3.10 | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@v4 | ||
| - name: Install dependencies | ||
| run: pip install -e . && pip install pytest | ||
| - name: Run tests | ||
| run: python -m pytest | ||
|
|
||
| test-py311: | ||
| runs-on: ubuntu-latest | ||
| needs: [test-py312] |
There was a problem hiding this comment.
The test job dependency chain is inverted. The jobs are configured so that test-py37 needs test-py38, which needs test-py39, which needs test-py310, which needs test-py311, which needs test-py312. This creates a reverse dependency chain where the oldest Python version depends on the newest one, causing jobs to run sequentially in reverse order (3.12 → 3.11 → 3.10 → 3.9 → 3.8 → 3.7).
If the intent is to run tests in parallel without dependencies, remove all needs statements. If the intent is to fail fast when a test fails on the latest version first, make test-py37 through test-py311 depend on test-py312 instead of creating a chain.
| raise InvalidArgument( | ||
| "The value provided in the request for atleast one parameter is invalid." | ||
| ) | ||
| msg = "The value provided in the request for atleast one parameter is invalid." |
There was a problem hiding this comment.
Spelling error: "atleast" should be "at least" (two words). This error message is shown to users when an invalid parameter value is provided.
| msg = "The value provided in the request for atleast one parameter is invalid." | |
| msg = "The value provided in the request for at least one parameter is invalid." |
|
|
||
| lint: build | ||
| @touch .env | ||
| @docker run --rm -t -u "${UID}:${GID}" -v "${PWD}:/code" --env-file .env python-amazon-paapi -c "python -m pre_commit run -a" |
There was a problem hiding this comment.
The command python -m pre_commit run -a is incorrect. The module name should use a hyphen, not an underscore: python -m pre-commit run -a. While the package is installed as pre-commit, the Python module uses underscores internally, but when running as a CLI tool via -m, it expects the hyphenated form or should be run directly as pre-commit run -a.
| @docker run --rm -t -u "${UID}:${GID}" -v "${PWD}:/code" --env-file .env python-amazon-paapi -c "python -m pre_commit run -a" | |
| @docker run --rm -t -u "${UID}:${GID}" -v "${PWD}:/code" --env-file .env python-amazon-paapi -c "pre-commit run -a" |
| run: pip install black | ||
| - name: Check code format | ||
| run: black --check --diff --color . | ||
| run: pip install mypy |
There was a problem hiding this comment.
The mypy job only installs mypy but not the package dependencies. Since mypy needs to type-check the amazon_paapi module which imports external dependencies (like those listed in setup.py), the job should also install the package with pip install -e . to ensure all imports can be resolved. Without this, mypy may fail with import errors.
| run: pip install mypy | |
| run: pip install -e . && pip install mypy |
No description provided.