Finish package: fix packaging, modernize, add public API, CI and docs

[dvejsada]

Make the package professional and installable as the basis for the Home Assistant integration and a future MCP server.

Packaging (blocking fixes)

• Include the rohlik_api.services subpackage in builds (was omitted, so installed wheels would ImportError)
• PEP 639 SPDX license metadata (license = "MIT") with setuptools>=77
• Single-source the version from rohlik_api.__version__ (dynamic in pyproject, read via importlib.metadata for the User-Agent)
• Target Python 3.13+, real author email, Beta status

API & typed models

• Add public RohlikAPI.login()/logout() plus user_id/address_id properties
• Service methods return typed dataclass models (Cart, SearchResults, ProductPrice, RecipeDetail, …) instead of raw dicts; raw passthrough endpoints (orders / delivery / account / get_data) still return decoded JSON
• Modern typing (PEP 604 unions, builtin generics), consistent exception chaining and %-style logging
• format_price helper removes duplicated price formatting

Tooling, tests & docs

• GitHub Actions CI (ruff, black, mypy, pytest) on Python 3.13
• ruff, black, mypy all green; 118 tests passing
• Rewrote example.py against the current API; updated README and PUBLISHING

Review follow-ups (addressed in 4c2b0c0)

• auth caches the real login response; logout resets cached user/address IDs
• removed duplicate credential validation; logout-on-close now logs at WARNING
• documented the "reads return None / writes raise" error convention in BaseService

[dvejsada]

@claude review

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[dvejsada]

@claude review

[claude]

Claude finished @dvejsada's task in 3m 3s —— View job

---

Code Review

• Gather context from PR description and changed files
• Review packaging and project configuration
• Review public API (__init__.py, client.py, auth.py)
• Review models and helpers
• Review services layer
• Review tests and CI
• Post comprehensive review findings

---

This is a solid modernization pass overall — the packaging fixes are correct, the service-based architecture is clean, and the typed models are a clear improvement. A few issues worth addressing before merging, from most to least impactful.

---

🔴 High: Python version claim vs. reality

The PR description says "Target Python 3.11+" and "CI on 3.11-3.13", but the actual code only requires/tests 3.13:

• pyproject.toml:10 — requires-python = ">=3.13"
• pyproject.toml:23-24 — classifiers only list 3.13 and 3.14
• .github/workflows/ci.yml:16 — matrix only includes "3.13"

The code itself would likely work on 3.10+ (all union types are guarded by from __future__ import annotations, and dataclass(slots=True) is 3.10+). Either lower requires-python to >=3.11 (or 3.10) and expand the CI matrix to match the stated goal, or update the PR description to say "Python 3.13+". As-is, the documentation misleads users who are on 3.11 or 3.12.

Fix this →

---

🟡 Medium: Inconsistent error contracts across services

Services handle errors in two incompatible ways:

• CartService.get_content() and CartService.delete_item() — raise APIRequestFailedError ✅
• ProductService.search(), get_ai_summary(), get_composition(), get_price() — return None silently ❌

Callers must know which pattern applies per service, which is fragile and forces if result is None: guards everywhere on the product service but not on the cart service. The BaseService._fetch_endpoint() helper (base.py:34-57) returns None on error but is barely used, since each service reimplements the same try/except block inline.

Recommend picking one convention. Raising is generally more Pythonic (callers can always catch if they want to ignore failures). If the None-return pattern is kept for read-only queries, it should at least be documented in the base class.

---

🟡 Medium: logout() doesn't clear _user_id/_address_id

auth.py:139 resets self._is_logged_in = False but leaves _user_id and _address_id with stale values. Combined with auth.py:101 (if not self._user_id:), a logout + re-login skips re-fetching these IDs from the fresh login response. If the same RohlikAPI instance is reused across sessions (e.g., in a long-running HA integration), and the user's address changes, the old address_id persists silently.

This is probably intentional for the HA use case, but should be called out in a code comment so the next reader doesn't try to "fix" it.

---

🟡 Medium: get_data() exposes a synthetic login dict

client.py:219:

result["login"] = await self._auth.login()

When already logged in, auth.login() returns the synthetic {"status": 200, "message": "Already logged in"} (auth.py:75) — not the real API response. Callers inspecting result["login"] for actual login data (user info, address, etc.) get nothing useful after the first call. The HA integration presumably depends on this method; it should either always return fresh login data or document that result["login"] is not reliable after initial auth.

---

🟢 Low: Redundant @pytest.mark.asyncio decorators

pyproject.toml:91 sets asyncio_mode = "auto", which means every async test function is automatically treated as an asyncio test. The @pytest.mark.asyncio decorators in the test files (test_client.py:87, test_services.py:50, etc.) are now redundant. They don't break anything, but they add noise and could confuse contributors.

---

🟢 Low: Duplicate credential validation

Both RohlikAPI.__init__ (client.py:66) and AuthManager.__init__ (auth.py:35) validate that username and password are non-empty. The RohlikAPI check fires first but is immediately superseded by the AuthManager check. Keep the validation in AuthManager only (the right owner), or remove the one from AuthManager if you want the facade to own validation. Having both creates a risk of them diverging.

---

🟢 Low: add_items() silently discards per-item errors

cart.py:63-69: individual product add failures are logged but swallowed — the caller only sees the list of successfully added IDs, with no indication of which items failed or why. Consider returning a more structured result (e.g., dict[int, Exception | None]) or at minimum raising after the loop if the success list is shorter than the input list.

---

🟢 Low: Best-effort logout logged at ERROR level

client.py:198-199:

except Exception as err:  # noqa: BLE001 - best-effort logout on close
    _LOGGER.error("Error during logout on close: %s", err)

The comment says "best-effort", which is WARNING territory. Using ERROR causes spurious alerts in HA/log monitoring setups when the session just expired before close() was called. Change to _LOGGER.warning(...).

---

✅ What's done well

• Packaging fixes are correct — [tool.setuptools.packages.find] include = ["rohlik_api*"] properly includes the services subpackage, and the PEP 639 license metadata is the right approach for setuptools>=77.
• HttpClient.client lazy init + re-creation on closed client is clean and handles the HA restart-without-explicit-close pattern well.
• format_price extraction removes the only meaningful code duplication.
• dataclass(slots=True) on all models is a nice memory efficiency win.
• from __future__ import annotations throughout means the PEP 604 union types are safe even if someone does backport to 3.10.
• Promoted product filtering in ProductService.search() (products.py:55-59) is a thoughtful UX detail to strip search noise.
• ensure_logged_in() at the top of every service method makes the public API session-safe without requiring callers to manually manage auth lifecycle.

[dvejsada]

Thanks for the review — addressed in 4c2b0c0.

• 🔴 Python version claim: standardized on 3.13 (the intended minimum for the HA integration) and corrected the PR description, which still said 3.11+. requires-python, classifiers and the CI matrix already target 3.13.
• 🟡 Inconsistent error contract: documented the convention in BaseService — read/optional fetches return None; critical/mutating ops raise APIRequestFailedError. Kept by design (rather than making every service raise) because the MCP server relies on None for graceful degradation.
• 🟡 logout() stale IDs: now resets user_id/address_id (and the cached login response) via a new _reset_session().
• 🟡 get_data() synthetic login: login() now caches the real login response and returns it on subsequent calls, so result["login"] is meaningful after the first auth.
• 🟢 Redundant @pytest.mark.asyncio: removed (asyncio_mode = "auto").
• 🟢 Duplicate credential validation: removed from RohlikAPI.__init__; AuthManager owns it (empty creds still raise ValueError).
• 🟢 logout-on-close log level: ERROR → WARNING.
• 🟢 add_items() swallows per-item errors: left as-is intentionally — it returns the list of successfully-added IDs, which the caller can diff against the input. Changing the return type would churn the just-stabilized contract and the MCP server's add_to_cart; happy to switch to a structured result later if you'd prefer.

118 tests green; ruff / black / mypy clean.

---

Generated by Claude Code

[dvejsada]

@claude review

---

Generated by Claude Code

[claude]

Claude finished @dvejsada's task in 2m 51s —— View job

---

Code Review (Re-review)

• Read CLAUDE.md and gather context
• Review diff since last review (commit 4c2b0c0)
• Verify previous feedback was addressed correctly
• Check for any new issues
• Post comprehensive review

---

All seven acted-upon items from the first review are correctly addressed. A few smaller issues remain worth noting.

---

✅ Previous Feedback — Verified

Finding	Status
Python version claim in PR description	✅ Updated to say 3.13+
logout() stale user_id/address_id	✅ _reset_session() clears both + cached response
get_data() synthetic login dict	✅ login() now caches and returns the real response
Duplicate credential validation	✅ Removed from RohlikAPI.__init__; AuthManager owns it
Best-effort logout logged at ERROR	✅ Changed to _LOGGER.warning (client.py:198)
Redundant @pytest.mark.asyncio	✅ Removed; asyncio_mode = "auto" handles it
Error contract inconsistency	✅ Documented in BaseService docstring (base.py:20-27)
add_items() swallows per-item errors	intentionally left (caller diffs input vs. returned list)

---

🟡 Medium: _fetch_endpoint logs optional failures at ERROR

base.py:63:

except httpx.HTTPError as err:
    _LOGGER.error("Error fetching %s: %s", error_context, err)
    return None

The docstring now correctly classifies these as "optional fetches" that return None for graceful degradation. But logging that at ERROR level will surface spurious alerts in HA/monitoring setups for expected transient failures (network hiccup, endpoint not configured). Should be _LOGGER.warning(...), consistent with the philosophy applied to close().

Fix this →

---

🟢 Low: Dead guards in login() after _reset_session()

auth.py:104,107:

if not self._user_id:
    self._user_id = data.get("user", {}).get("id")

if not self._address_id:
    ...

These guards made sense before when login could be called twice without logout. Now that _reset_session() always zeros out _user_id and _address_id before a real login can occur (login returns early if _is_logged_in is True), these if not checks are always True and add no protection. They're harmless, but the try/except AttributeError wrapped around the address lookup is the only reason they exist — and that handler correctly fires when data["address"] is explicitly null in the response. Consider simplifying to:

self._user_id = data.get("user", {}).get("id")
self._address_id = (data.get("address") or {}).get("id")

This removes the dead if not guards and replaces try/except AttributeError with an explicit or {} guard that handles the same null-address edge case more readably.

---

🟢 Low: Double _ensure_logged_in() in get_next_slots

delivery.py:45-54:

async def get_next_slots(self, ...):
    await self._ensure_logged_in()          # ← first call
    ...
    return await self._fetch_endpoint(url, "next delivery slots")  # ← _fetch_endpoint also calls it

_fetch_endpoint (base.py:56) calls _ensure_logged_in() itself. The explicit call at line 45 is redundant (it's idempotent, so no bug). Since the explicit call is only there to access self._auth.user_id before the _fetch_endpoint call, it's not wrong — but a comment or removing the explicit call would reduce noise.

---

🟢 Low: mypy strictness is lenient for a Typing :: Typed package

pyproject.toml:83-84:

disallow_untyped_defs = false
disallow_incomplete_defs = false

The package ships py.typed and advertises Typing :: Typed, which signals to users (and type checkers) that all public symbols are annotated. With these mypy flags off, partially-typed public functions can slip through silently. Consider enabling disallow_untyped_defs = true at least for the rohlik_api package (not tests) to keep that promise — or document that type coverage is partial.

---

✅ What's solid

• _reset_session() is clean and its docstring explains the deliberate decision to clear IDs on logout so a reused instance picks up an address change.
• The cached-login-response approach (_login_response returned on second call) correctly makes get_data()["login"] meaningful without a redundant network round-trip.
• Test additions in 4c2b0c0 (test_login_caches_response_and_extracts_ids, test_logout_resets_session_state) directly exercise the invariants that were previously missing.
• The format_price usage in ProductSearchResult.from_api is correct — the search API returns price as {"full": ..., "currency": ...}, which the helper expects.

---

Overall: The PR is in good shape. The two medium/low items above are improvements, not blockers. The _fetch_endpoint logging level is the most practical change before merge if HA integration will run this in production.

[dvejsada]

Re-review items addressed in 4721ddf:

• 🟡 _fetch_endpoint (and other optional reads) logged at ERROR: moved all return-None read paths to WARNING (base / products / recipes / orders / delivery, plus the swallowed per-item add_items failure). Raise paths (cart.get_content, cart.delete_item, account.get_shopping_list) stay ERROR.
• 🟢 Dead guards in login(): removed the if not self._user_id/_address_id guards (always true now that _reset_session() zeroes them) and replaced try/except AttributeError with an explicit (data.get("address") or {}).get("id") for the null-address case.
• 🟢 Double _ensure_logged_in() in get_next_slots: kept (it must populate user_id/address_id before building the URL) and added a comment explaining why.
• 🟢 Lenient mypy for a Typing :: Typed package: enabled disallow_untyped_defs and disallow_incomplete_defs for the package (tests excluded). Package is clean under strict mode.

118 tests green; ruff, black and strict mypy all clean. CI was green on the previous push.

---

Generated by Claude Code

[dvejsada]

@claude review

---

Generated by Claude Code

[claude]

Claude finished @dvejsada's task in 1m 20s —— View job

---

Code Review (Third Pass)

• Review diff since last review (commit 4721ddf)
• Verify previous feedback was addressed correctly
• Check for any new issues
• Post comprehensive review

---

All three items from the second review are correctly addressed. No new issues found.

✅ Previous Feedback — Verified

Finding	Status
_fetch_endpoint logs optional failures at ERROR	✅ Changed to WARNING (base.py:64)
Dead if not guards in login()	✅ Removed; (data.get("address") or {}).get("id") handles null-address; missing address now logs at DEBUG instead of ERROR
Double _ensure_logged_in() in get_next_slots	✅ Documented with comment explaining why the explicit call is necessary
Lenient mypy for a Typing :: Typed package	✅ disallow_untyped_defs = true and disallow_incomplete_defs = true enabled; tests excluded via exclude = ["^tests/"]

Commit quality

The 4721ddf commit is clean: 8 files, 26 additions, 25 deletions. Every change is a direct response to review feedback, there are no unrelated edits, and the commit message accurately describes all changes. The 118-test / ruff / black / strict-mypy clean signal holds.

Ready to merge ✅

This PR is in excellent shape. Five rounds of review have tightened a solid initial modernization into something production-ready. All blocking and advisory findings are resolved.