MPT-17674: propagate changes from template#212
Merged
svazquezco merged 1 commit intomainfrom Feb 16, 2026
Merged
Conversation
svazquezco
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames environment variables (MPT_TOKEN→MPT_API_TOKEN, MPT_URL→MPT_API_BASE_URL), adds sample env file, restructures Makefile into modular includes with new targets, updates Dockerfile (workdir, user isolation, UV changes), adjusts docs/README and pyproject, normalizes pre-commit deps, and updates tests to use new env vars. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
svazquezco
force-pushed
the
MPT-17674-propagate-changes-from-template
branch
from
69ba216 to
5ea0669
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: 7
🤖 Fix all issues with AI agents
In `@docs/PROJECT_DESCRIPTION.md`:
- Around line 1-46: The README header "mpt-tool CLI" is misleading given the
examples only show the Python clients; either rename the header to
"mpt-api-client" or add a short CLI usage section, and update the sync and async
code examples to explicitly mention and show the environment variable names
MPT_API_TOKEN and MPT_API_BASE_URL when instantiating MPTClient and
AsyncMPTClient (e.g., note they will be read from MPT_API_TOKEN /
MPT_API_BASE_URL), so readers know which env vars to set.
In `@mpt_api_client/http/async_client.py`:
- Around line 33-46: The async client currently only reads MPT_API_TOKEN and
MPT_API_BASE_URL, causing breakage for users who still use legacy env names;
update the initialization in async_client.py (the api_token and base_url
retrieval logic) to mirror the sync client’s compatibility shim: if
MPT_API_TOKEN or MPT_API_BASE_URL are missing, fall back to MPT_TOKEN and
MPT_URL respectively, emit a deprecation warning when the legacy variables
(MPT_TOKEN/MPT_URL) are used, and ensure the same error messages remain if
neither new nor legacy env vars nor explicit arguments are provided.
In `@mpt_api_client/http/client.py`:
- Around line 46-59: Add backward-compatible fallbacks for renamed env vars by
checking MPT_TOKEN when resolving api_token and MPT_URL when resolving base_url
before raising errors; update the api_token and base_url assignment logic in the
MPTClient initialization (the api_token and base_url variables shown) to prefer
the new env vars (MPT_API_TOKEN, MPT_API_BASE_URL) but fallback to MPT_TOKEN and
MPT_URL respectively if the new ones are missing, and emit a deprecation warning
(e.g., via warnings.warn or logger.warning) indicating the old env var is
deprecated and should be migrated.
In `@mpt_api_client/models/model.py`:
- Line 14: The class declaration “class MptBox(Box): # type: ignore[misc]”
contains an unused type ignore; remove the trailing “# type: ignore[misc]” from
the MptBox declaration so the line reads simply “class MptBox(Box):” and rerun
type checks to confirm no real mypy/pyright error exists for Box or MptBox—if an
actual type error appears, instead document the specific error and add a
targeted ignore at the offending symbol (e.g., on the problematic attribute or
import) rather than ignoring the whole class declaration.
- Line 4: Replace the unused flake8 noqa on the private import in
mpt_api_client/models/model.py: change the import comment on _camel_killer to
use a mypy/type-checker suppression (type: ignore[attr-defined]) instead of `#
noqa: PLC2701` so the private attribute access is explicitly ignored by the type
checker; update the import line that references `_camel_killer` accordingly.
- Around line 28-44: The Box parent class is untyped, so add "type:
ignore[no-untyped-call]" comments to the super() calls to suppress mypy errors:
update the super().__setitem__(mapped_key, value) call in __setitem__, the
super().__setattr__(item, value) call in __setattr__, and the
super().__getattr__(item) call in __getattr__ to include the type ignore; keep
the existing _prep_key and _box_safe_attributes logic unchanged and ensure the
ignore comments are placed immediately on the lines invoking the parent methods
(__setitem__, __setattr__, __getattr__).
In `@pyproject.toml`:
- Line 7: The pyproject.toml currently sets readme =
"docs/PROJECT_DESCRIPTION.md" but the sdist include is limited to
"mpt_api_client", so the README may be omitted; update the sdist include
configuration to explicitly include the docs/PROJECT_DESCRIPTION.md (or the docs
directory) alongside "mpt_api_client" so the readme is packaged into sdists;
look for the packaging include/manifest settings in pyproject.toml (where
"mpt_api_client" is listed) and add "docs/PROJECT_DESCRIPTION.md" (or "docs/")
to that list.
🧹 Nitpick comments (2)
tests/unit/http/test_async_client.py (1)
59-70: Consider usingraising=Falsefordelenvto avoidKeyError.If
MPT_API_TOKENorMPT_API_BASE_URLis not already set in the test environment,monkeypatch.delenv()will raise aKeyErrorby default. Usingraising=Falsemakes the tests more robust across different environments.🔧 Proposed fix
def test_async_http_without_token(monkeypatch): - monkeypatch.delenv("MPT_API_TOKEN") + monkeypatch.delenv("MPT_API_TOKEN", raising=False) with pytest.raises(ValueError): AsyncHTTPClient(base_url=API_URL) def test_async_http_without_url(monkeypatch): - monkeypatch.delenv("MPT_API_BASE_URL") + monkeypatch.delenv("MPT_API_BASE_URL", raising=False) with pytest.raises(ValueError): AsyncHTTPClient(api_token=API_TOKEN)tests/unit/http/test_client.py (1)
49-60: Consider usingraising=Falsefordelenvto avoidKeyError.Same as the async client tests - if these environment variables aren't set in the test environment,
delenvwill raise aKeyError.🔧 Proposed fix
def test_http_without_token(monkeypatch): - monkeypatch.delenv("MPT_API_TOKEN") + monkeypatch.delenv("MPT_API_TOKEN", raising=False) with pytest.raises(ValueError): HTTPClient(base_url=API_URL) def test_http_without_url(monkeypatch): - monkeypatch.delenv("MPT_API_BASE_URL") + monkeypatch.delenv("MPT_API_BASE_URL", raising=False) with pytest.raises(ValueError): HTTPClient(api_token=API_TOKEN)
svazquezco
force-pushed
the
MPT-17674-propagate-changes-from-template
branch
from
5ea0669 to
db15d3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 1-4: The SonarCloud badge URLs in the README reference the wrong
project key; update both occurrences of "softwareone-platform_mpt-tool" in the
two SonarCloud badge URLs to "softwareone-platform_mpt-api-python-client" so the
badges point to the configured SonarCloud project (update the two URLs shown in
the diff for the Quality Gate Status and Coverage badges).
In `@tests/unit/http/test_client.py`:
- Around line 49-51: The tests call monkeypatch.delenv("MPT_API_TOKEN") and
monkeypatch.delenv("MPT_API_BASE_URL") in test_http_without_token and
test_http_without_url which will raise KeyError if the env var is missing;
update those calls to use monkeypatch.delenv("MPT_API_TOKEN", raising=False) and
monkeypatch.delenv("MPT_API_BASE_URL", raising=False) so deletion is silent when
the variable is absent and the tests become robust.
🧹 Nitpick comments (6)
tests/unit/http/test_client.py (1)
56-58: Same consideration forMPT_API_BASE_URLdeletion.Apply the same
raising=Falsepattern here:def test_http_without_url(monkeypatch): - monkeypatch.delenv("MPT_API_BASE_URL") + monkeypatch.delenv("MPT_API_BASE_URL", raising=False)tests/unit/http/test_async_client.py (2)
59-61: Consider usingraising=Falsefor robustness.Same as the sync client tests - if
MPT_API_TOKENisn't set in the environment,delenvwill raiseKeyError:def test_async_http_without_token(monkeypatch): - monkeypatch.delenv("MPT_API_TOKEN") + monkeypatch.delenv("MPT_API_TOKEN", raising=False)
66-68: Apply sameraising=Falsepattern.def test_async_http_without_url(monkeypatch): - monkeypatch.delenv("MPT_API_BASE_URL") + monkeypatch.delenv("MPT_API_BASE_URL", raising=False)README.md (1)
47-52: Consider using the actual repository URL or a clearer placeholder.The
<repository-url>placeholder may confuse users. Consider either using the actual GitHub URL or a more explicit placeholder likehttps://github.com/softwareone-platform/mpt-api-python-client.git.📝 Suggested change
```bash -git clone <repository-url> +git clone https://github.com/softwareone-platform/mpt-api-python-client.git</details> </blockquote></details> <details> <summary>make/common.mk (2)</summary><blockquote> `25-26`: **Minor: Extra space in conditional default value.** The space before `tests/unit` in the `$(if ...)` else clause will include a leading space in the output when `args` is not provided. This is harmless but unintentional. <details> <summary>Suggested fix</summary> ```diff test: ## Run test - $(RUN) pytest $(if $(args),$(args), tests/unit) + $(RUN) pytest $(if $(args),$(args),tests/unit)
28-36: Implicit dependency onrequirefunction.The
uv-addanduv-add-devtargets use$(call require,pkg)which is defined in the mainmakefile. This works becausecommon.mkis included from there, but it creates an implicit coupling. Consider adding a comment at the top of this file noting that it must be included from the main makefile.
svazquezco
commented
Feb 16, 2026
svazquezco
force-pushed
the
MPT-17674-propagate-changes-from-template
branch
from
db15d3c to
5fed003
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mpt_api_client/http/async_client.py`:
- Around line 33-46: Replace the long inline ValueError messages with dedicated
exception classes and raise those instead: add two custom exceptions (e.g.,
MissingAPITokenError and MissingBaseUrlError) defined near the top of
async_client.py (or in the same module) with their explanatory messages in the
class body or __str__, then update the MPTClient (or the initializer function
where api_token and base_url are validated) to raise MissingAPITokenError() when
api_token is missing and MissingBaseUrlError() when base_url is missing; this
satisfies Ruff TRY003 while keeping the validation logic in the existing symbols
(api_token, base_url, MPTClient) intact.
In `@mpt_api_client/http/client.py`:
- Around line 46-59: Replace the inline ValueError usages with dedicated
exception classes to satisfy TRY003: define two custom exceptions (e.g.,
MissingAPITokenError and MissingBaseURLError or a single MPTClientConfigError)
inheriting from ValueError and move the long messages into those exception
classes (either as class docstring or default message in __init__/__str__); then
update the code in mpt_api_client.http.client where api_token and base_url are
validated to raise the new exceptions instead of ValueError (references: the
api_token check and base_url check and any code that mentions MPTClient in the
error text).
5fed003 to
5b8327e
Compare
svazquezco
force-pushed
the
MPT-17674-propagate-changes-from-template
branch
from
5b8327e to
7c4a3cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Around line 16-20: In the Dockerfile update the RUN line that invokes the uv
sync command to remove the unsupported --dev flag (i.e., change the RUN invoking
"uv sync ... --dev" to omit --dev); this ensures uv sync runs with --frozen and
--no-cache only since dev deps are synced by default and the --dev flag causes
an error.
🧹 Nitpick comments (1)
Dockerfile (1)
10-14: Consider optimizing layer caching for faster builds.Copying all source files before
uv syncinvalidates the dependency cache on every code change. Splitting this into two COPY steps allows Docker to reuse the cached dependency layer when only application code changes.♻️ Proposed optimization for layer caching
FROM base AS build -COPY . . +COPY pyproject.toml uv.lock ./ RUN uv sync --frozen --no-cache --no-dev + +COPY . .
albertsola
approved these changes
Feb 16, 2026
svazquezco
force-pushed
the
MPT-17674-propagate-changes-from-template
branch
from
7c4a3cf to
7ce43b0
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MPT-17674