fix: address non-blocking reviewer nits from #178, #179, and #157#194
fix: address non-blocking reviewer nits from #178, #179, and #157#194mimran-khan wants to merge 2 commits into
Conversation
Add DEFAULT_MODEL and SLOT_DEFAULTS as ClassVar declarations to ModelMetadataProvider Protocol so callers no longer need type: ignore[attr-defined] when accessing provider attributes. Add validate_base_url() helper that warns on malformed (non-http/https or missing host) base URLs at model creation time, catching operator misconfigurations early without raising.
mohgupta-ship-it
left a comment
mohgupta-ship-it
left a comment
There was a problem hiding this comment.
PR #194 is mostly good, but I would ask for one small fix before merging.
The real reason it’s needed: it makes the provider interface honest. ModelMetadataProvider users already access DEFAULT_MODEL and SLOT_DEFAULTS, but the protocol did not declare them, so type checkers needed a suppressing comment. The base URL warning is also useful operator polish: bad OPENAI_BASE_URL / provider URLs fail earlier with a clear warning instead of later with a vague HTTP/client error.
One issue: the PR adds the protocol attributes but does not remove the old ignore in src/skillspector/constants.py:36.
_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL # type: ignore[attr-defined]With the PR applied, that ignore becomes unused. I verified:
uv run --extra dev mypy src/skillspector/constants.py src/skillspector/providers/base.py
src/skillspector/constants.py:36: error: Unused "type: ignore" comment
So the fix is just:
_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODELOther results: targeted tests pass, 46 passed, 9 skipped; ruff passes on changed files.
Small nit: assert hasattr(ModelMetadataProvider, "__protocol_attrs__") or True in the new test is a no-op and should be removed. Also the PR mentions #157, but that SVG change is not actually in this PR, so the wording is a little confusing.
Verdict: good small cleanup PR, genuinely useful, but merge after removing the stale type: ignore and the no-op test assertion.
Remove unused # type: ignore[attr-defined] from constants.py now that ModelMetadataProvider declares DEFAULT_MODEL. Remove no-op `assert hasattr(...) or True` from test.
|
Thanks @mohgupta-ship-it — all three items addressed:
46 passed, 9 skipped; ruff clean. Ready for re-review. |
2 participants
Summary
Addresses non-blocking reviewer suggestions from two approved PRs:
ModelMetadataProviderProtocol now declaresDEFAULT_MODELandSLOT_DEFAULTSasClassVarattributes, eliminating# type: ignore[attr-defined]suppressions when callers access provider attributes. The staletype: ignoreinconstants.pyis also removed.validate_base_url()helper warns on malformed base URLs (non-http/https scheme, missing host) at model creation time — catches operator misconfigurations early without raisingChanges
src/skillspector/providers/base.py— addDEFAULT_MODEL: ClassVar[str]andSLOT_DEFAULTS: ClassVar[dict[str, str]]toModelMetadataProvidersrc/skillspector/providers/chat_models.py— addvalidate_base_url(), called fromcreate_openai_compatible_chat_modelsrc/skillspector/constants.py— remove stale# type: ignore[attr-defined]tests/unit/test_reviewer_nits.py— 10 tests: Protocol attribute declarations, existing provider conformance, URL validation (valid/invalid/edge cases)Test plan
pytest tests/unit/test_reviewer_nits.py -v)DEFAULT_MODELandSLOT_DEFAULTStype: ignore[attr-defined]removed fromconstants.pyNonebase URL produces no warningsruff check)