MPT-17028: add airtable_client and mpt_client mixins #18
Conversation
📝 WalkthroughWalkthroughAdds two mixins (AirtableAPIClientMixin, MPTAPIClientMixin) that provide cached API clients from environment variables, exports them from the mixins package, adds unit tests, and updates mypy/pre-commit and pyproject dependencies to include Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
svazquezco
force-pushed
the
MPT-17028-implement-mixins
branch
from
0adeef6 to
92c5458
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.pre-commit-config.yaml:
- Around line 37-40: The project imports mpt_api_client and pyairtable (see
mpt_tool/commands/mixins/mpt_client.py and
mpt_tool/commands/mixins/airtable_client.py) but they are not declared in
pyproject.toml, causing CI mypy failures; add both packages to the declared
dependencies in pyproject.toml (the same dependency section used by your CI
installer—e.g., [project]/dependencies or the poetry/poetry-core dependencies
group) so CI can install and type-check them, ensuring names match the installed
package names ("mpt_api_client" and "pyairtable") and bump any lockfile if
applicable.
In `@tests/commands/mixins/test_airtable_client.py`:
- Around line 6-11: The test test_airtable_client should not instantiate or
evaluate the cached_property airtable_client; change the check to inspect the
class attribute instead (e.g., use hasattr(AirtableAPIClientMixin,
"airtable_client") or hasattr(type(mixin), "airtable_client")) and remove the
unused mocker fixture argument from the test signature so the property is not
triggered and the test does not depend on AIRTABLE_API_KEY.
In `@tests/commands/mixins/test_mpt_client.py`:
- Around line 6-11: Update the test_mpt_client_mixin to avoid evaluating the
cached_property: remove the unused mocker fixture and assert the attribute
exists on the class rather than the instance (e.g., use
hasattr(MPTAPIClientMixin, "mpt_client") or inspect type(mixin) for
"mpt_client") so the property is not invoked; reference the test function
test_mpt_client_mixin and the MPTAPIClientMixin class and its mpt_client
attribute when making the change.
- Around line 30-39: The test test_mpt_client_is_cached incorrectly sets the
environment variable name and declares an unused fixture: change
monkeypatch.setenv("MPT_BASE_URL", ...) to
monkeypatch.setenv("MPT_API_BASE_URL", "https://test.com") so MPTAPIClientMixin
reads the correct base URL, and remove the unused mocker parameter from the test
signature so the function is defined as def
test_mpt_client_is_cached(monkeypatch):; keep the rest (creating
MPTAPIClientMixin and asserting the cached mpt_client instances) unchanged.
- Around line 14-27: The test test_mpt_client_env_not_set expects the wrong
error message text; update the pytest.raises match to the plural form the mixin
actually raises (e.g., change "MPT API token and base URL must be set in env
variable" to "MPT API token and base URL must be set in env variables") so the
ValueError from MPTAPIClientMixin.mpt_client matches correctly.
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.pre-commit-config.yamlmpt_tool/commands/mixins/__init__.pympt_tool/commands/mixins/airtable_client.pympt_tool/commands/mixins/mpt_client.pytests/commands/mixins/__init__.pytests/commands/mixins/test_airtable_client.pytests/commands/mixins/test_mpt_client.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: All configuration must be provided via environment variables; do not hardcode configuration values
Use type annotations (PEP 484) in Python files, except in thetests/folder
All public functions, methods, and classes must include Google-style docstrings
Do not add inline comments; rely on clear code and docstrings instead
Function and variable names must be explicit and intention-revealing
Follow PEP 8 unless explicitly overridden by ruff
Files:
mpt_tool/commands/mixins/__init__.pympt_tool/commands/mixins/airtable_client.pytests/commands/mixins/test_mpt_client.pytests/commands/mixins/test_airtable_client.pympt_tool/commands/mixins/mpt_client.py
⚙️ CodeRabbit configuration file
**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.
Files:
mpt_tool/commands/mixins/__init__.pympt_tool/commands/mixins/airtable_client.pytests/commands/mixins/test_mpt_client.pytests/commands/mixins/test_airtable_client.pympt_tool/commands/mixins/mpt_client.py
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
mpt_tool/commands/mixins/__init__.pympt_tool/commands/mixins/airtable_client.pytests/commands/mixins/test_mpt_client.pytests/commands/mixins/test_airtable_client.pympt_tool/commands/mixins/mpt_client.py
**/test_*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/test_*.py: Tests must be written as functions, not classes, and must use thetest_prefix
Follow AAA (Arrange - Act - Assert) pattern strictly in test functions
Do not useifstatements or branching logic inside tests
Prefer fixtures over mocks whenever possible in tests
Avoid duplicating test logic; extract shared setup into fixtures
Usemockeronly when mocking is unavoidable in tests; never useunittest.mockdirectly
Always usespecorautospecwhen mocking in tests
Use@pytest.mark.parametrizewhen testing permutations of the same behavior
Files:
tests/commands/mixins/test_mpt_client.pytests/commands/mixins/test_airtable_client.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T13:54:01.365Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:54:01.365Z
Learning: When reviewing Ruff lint violations, first check the project's pyproject.toml under [tool.ruff.lint].ignore to see which rules are explicitly ignored. Do not report violations for rules listed in that ignore list. Apply this guidance across all repositories in the softwareone-platform organization that use the same Ruff configuration.
Applied to files:
mpt_tool/commands/mixins/__init__.pympt_tool/commands/mixins/airtable_client.pytests/commands/mixins/test_mpt_client.pytests/commands/mixins/test_airtable_client.pympt_tool/commands/mixins/mpt_client.py
📚 Learning: 2026-01-05T16:00:35.764Z
Learnt from: CR
Repo: softwareone-platform/mpt-tool PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-05T16:00:35.764Z
Learning: Applies to **/test_*.py : Use `mocker` only when mocking is unavoidable in tests; never use `unittest.mock` directly
Applied to files:
tests/commands/mixins/test_mpt_client.py
📚 Learning: 2026-01-09T16:49:37.035Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 10
File: tests/commands/test_base.py:19-24
Timestamp: 2026-01-09T16:49:37.035Z
Learning: In tests that access protected attributes (e.g., _type) from command classes, add a per-line noqa directive to suppress wemake-python-styleguide's protected attribute usage violation: # noqa: WPS437. Ruff may flag unused (RUF100) because it doesn't recognize wemake rules, but keep the noqa comment to maintain flake8/wemake compliance. Apply this guideline to tests under tests/commands (and similar test modules) as needed.
Applied to files:
tests/commands/mixins/test_mpt_client.pytests/commands/mixins/test_airtable_client.py
🧬 Code graph analysis (3)
mpt_tool/commands/mixins/__init__.py (2)
mpt_tool/commands/mixins/airtable_client.py (2)
airtable_client(14-28)AirtableAPIClientMixin(7-28)mpt_tool/commands/mixins/mpt_client.py (2)
mpt_client(15-30)MPTAPIClientMixin(7-30)
tests/commands/mixins/test_mpt_client.py (1)
mpt_tool/commands/mixins/mpt_client.py (2)
MPTAPIClientMixin(7-30)mpt_client(15-30)
tests/commands/mixins/test_airtable_client.py (1)
mpt_tool/commands/mixins/airtable_client.py (2)
AirtableAPIClientMixin(7-28)airtable_client(14-28)
🪛 GitHub Actions: PR build and merge
mpt_tool/commands/mixins/airtable_client.py
[error] 4-4: Cannot find implementation or library stub for module named "pyairtable" [import-not-found]
mpt_tool/commands/mixins/mpt_client.py
[error] 4-4: Cannot find implementation or library stub for module named "mpt_api_client" [import-not-found]
🪛 Ruff (0.14.11)
mpt_tool/commands/mixins/airtable_client.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
tests/commands/mixins/test_mpt_client.py
6-6: Unused function argument: mocker
(ARG001)
27-27: Unused noqa directive (unknown: WPS122)
Remove unused noqa directive
(RUF100)
30-30: Unused function argument: mocker
(ARG001)
tests/commands/mixins/test_airtable_client.py
6-6: Unused function argument: mocker
(ARG001)
21-21: Unused noqa directive (unknown: WPS122)
Remove unused noqa directive
(RUF100)
mpt_tool/commands/mixins/mpt_client.py
28-28: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
mpt_tool/commands/mixins/mpt_client.py (1)
7-30: LGTM: clean lazy client initialization with env validation.mpt_tool/commands/mixins/airtable_client.py (1)
7-28: LGTM: straightforward cached client creation.tests/commands/mixins/test_airtable_client.py (1)
14-21: LGTM: env validation test is clear and focused.mpt_tool/commands/mixins/__init__.py (1)
1-4: LGTM!Clean package initialization following Python best practices. The imports match the class definitions in the source modules, and
__all__correctly exports the public API of the mixins subpackage.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
svazquezco
force-pushed
the
MPT-17028-implement-mixins
branch
3 times, most recently
from
339d03a to
6fe2f55
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/commands/mixins/test_airtable_client.py`:
- Around line 16-19: The test currently assigns the property to a dummy variable
to trigger mixin.airtable_client inside pytest.raises which forces a redundant
noqa; instead, remove the `_ =` assignment and directly access the property
inside the raises block (i.e., evaluate mixin.airtable_client without
assignment) so the ValueError is still asserted and the unused `# noqa: WPS122`
is no longer necessary; update the test to call/access mixin.airtable_client
directly within the pytest.raises context.
♻️ Duplicate comments (2)
tests/commands/mixins/test_mpt_client.py (2)
24-25: Drop the unusednoqanow that the error message is fixed.
The earlier message mismatch is resolved, but the# noqa: WPS122now triggers RUF100. Use a direct access to avoid the assignment.Proposed fix
- with pytest.raises(ValueError, match="MPT API token and base URL must be set in env variables"): - _ = mixin.mpt_client # noqa: WPS122 + with pytest.raises(ValueError, match="MPT API token and base URL must be set in env variables"): + mixin.mpt_client
28-31: Fix the env var name used by the cached-client test.
Line 30 setsMPT_BASE_URL, but the mixin readsMPT_API_BASE_URL, which is why CI fails.Proposed fix
- monkeypatch.setenv("MPT_BASE_URL", "https://test.com") + monkeypatch.setenv("MPT_API_BASE_URL", "https://test.com")
svazquezco
force-pushed
the
MPT-17028-implement-mixins
branch
from
6fe2f55 to
a0b9a27
Compare
|
Closes MPT-17028