Conversation
|
@cubic-dev-ai review this |
@zombyacoff I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 37 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/faceit/resources/aggregator.py">
<violation number="1" location="src/faceit/resources/aggregator.py:91">
P1: `NullCallable().__call__` returns `self` (truthy), so using it as `__exit__` will silently suppress all exceptions inside a `with` block. The context manager protocol interprets a truthy `__exit__` return as "suppress the exception". Consider using a dedicated no-op that returns `None`, or override `__exit__` with a proper method.</violation>
</file>
<file name="src/faceit/models/players/general.py">
<violation number="1" location="src/faceit/models/players/general.py:43">
P2: `SkillLevel.get_level()` can return `None`, which would replace a valid integer `skill_level` with `None` and cause a validation error on a non-optional field. Guard against this by only assigning the result when it's not `None`.</violation>
</file>
<file name="src/faceit/faceit.py">
<violation number="1" location="src/faceit/faceit.py:10">
P1: `@deprecated` on `BaseFaceit` causes a `DeprecationWarning` to be emitted at import time whenever `Faceit` or `AsyncFaceit` subclass definitions are evaluated. Every user who does `import faceit` will see spurious deprecation warnings they cannot suppress.
Since `BaseFaceit` is an internal class not meant for external subclassing, remove the `@deprecated` decorator from it. The `@deprecated` on `Faceit`/`AsyncFaceit` is sufficient to signal deprecation to static type checkers.</violation>
</file>
<file name="src/faceit/models/item_page.py">
<violation number="1" location="src/faceit/models/item_page.py:133">
P2: `typing.Generator[_T]` requires 3 type parameters on Python < 3.13 (this project supports >=3.8). The previous `typing.Iterator[_T]` was correct, or use the full form `typing.Generator[_T, None, None]` to match the pattern used elsewhere in this codebase (e.g., `pagination.py`).</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review bcfaf02 |
@zombyacoff I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 39 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/faceit/models/item_page.py">
<violation number="1" location="src/faceit/models/item_page.py:162">
P1: `reversed(self)` inside `__reversed__` causes infinite recursion. Use `reversed(self.items)` to reverse the underlying tuple directly.</violation>
</file>
<file name="src/faceit/models/players/general.py">
<violation number="1" location="src/faceit/models/players/general.py:50">
P2: Replace `assert` with an explicit `raise`. `assert` statements are removed when Python runs with `-O`/`-OO`, so this invariant check silently disappears in optimized mode.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| assert resolved is not None, ( | ||
| "`resolved` cannot be None because `game_id` was already validated " | ||
| "to be present in `ELO_THRESHOLDS`" | ||
| ) |
There was a problem hiding this comment.
P2: Replace assert with an explicit raise. assert statements are removed when Python runs with -O/-OO, so this invariant check silently disappears in optimized mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/faceit/models/players/general.py, line 50:
<comment>Replace `assert` with an explicit `raise`. `assert` statements are removed when Python runs with `-O`/`-OO`, so this invariant check silently disappears in optimized mode.</comment>
<file context>
@@ -13,19 +14,51 @@
+ and not isinstance(skill_lvl, SkillLevel)
+ ):
+ resolved = SkillLevel.get_level(game_id, skill_lvl)
+ assert resolved is not None, (
+ "`resolved` cannot be None because `game_id` was already validated "
+ "to be present in `ELO_THRESHOLDS`"
</file context>
| assert resolved is not None, ( | |
| "`resolved` cannot be None because `game_id` was already validated " | |
| "to be present in `ELO_THRESHOLDS`" | |
| ) | |
| if resolved is None: | |
| raise RuntimeError( | |
| "`resolved` cannot be None because `game_id` was already validated " | |
| "to be present in `ELO_THRESHOLDS`" | |
| ) |
There was a problem hiding this comment.
Не имеет смысла, т.к. я здесь просто даю знать стат. типизаторам и разработчикам, что все под контролем
@cubic-dev-ai
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
There was a problem hiding this comment.
Понял: здесь assert служит для инварианта/узкого типа для статических типизаторов, а не как рантайм‑проверка. Учту это в будущих ревью.
|
@cubic-dev-ai review 851d780 |
@zombyacoff I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 41 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/faceit/http/helpers.py">
<violation number="1" location="src/faceit/http/helpers.py:84">
P2: `str(other)` succeeds for virtually all Python objects (int, None, list, etc.), so the `except (TypeError, ValueError)` block is practically unreachable and `UnsupportedOperationTypeError` will never be raised for common misuse. If someone accidentally passes `None` or an `int`, this silently builds a malformed endpoint (e.g. `".../None"`) instead of failing fast.
If the intent is to accept any stringable object, update `EndpointParam` to reflect that. If only `str` and `Endpoint` are valid, restore the explicit `isinstance(other, str)` check before calling `self.add()`.</violation>
</file>
<file name="src/faceit/models/players/general.py">
<violation number="1" location="src/faceit/models/players/general.py:185">
P2: `total_sniper_kills` field is present in the old `LifetimeStats` and in the sibling `CSMapStats` model, but was dropped from `CSLifetimeStats`. This looks like an accidental omission during the refactor — API consumers lose access to this stat.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| int, Field(alias="Total Rounds with extended stats") | ||
| int, Field(0, alias="Total Rounds with extended stats") | ||
| ] | ||
| total_utility_count: Annotated[int, Field(0, alias="Total Utility Count")] |
There was a problem hiding this comment.
P2: total_sniper_kills field is present in the old LifetimeStats and in the sibling CSMapStats model, but was dropped from CSLifetimeStats. This looks like an accidental omission during the refactor — API consumers lose access to this stat.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/faceit/models/players/general.py, line 185:
<comment>`total_sniper_kills` field is present in the old `LifetimeStats` and in the sibling `CSMapStats` model, but was dropped from `CSLifetimeStats`. This looks like an accidental omission during the refactor — API consumers lose access to this stat.</comment>
<file context>
@@ -108,135 +142,142 @@ class Tournament(BaseModel):
- int, Field(alias="Total Rounds with extended stats")
+ int, Field(0, alias="Total Rounds with extended stats")
]
+ total_utility_count: Annotated[int, Field(0, alias="Total Utility Count")]
+ total_utility_damage: Annotated[int, Field(0, alias="Total Utility Damage")]
+ total_utility_successes: Annotated[int, Field(0, alias="Total Utility Successes")]
</file context>
| total_utility_count: Annotated[int, Field(0, alias="Total Utility Count")] | |
| total_sniper_kills: Annotated[int, Field(0, alias="Total Sniper Kills")] | |
| total_utility_count: Annotated[int, Field(0, alias="Total Utility Count")] |
Replace try/except str(other) with isinstance(other, str) in both __truediv__ and __itruediv__ so that non-str, non-Endpoint values (e.g. None, int) raise UnsupportedOperationTypeError instead of silently building malformed endpoints.
…tors" This reverts commit a76e446.
Summary by cubic
Removed the old facade entry points and made
SyncDataResourceandAsyncDataResourcethe primary API. This refactor simplifies client setup, improves pagination typing, adds automaticSkillLevelconversion, makesSkillLevelcomparable, standardizes retries and errors aroundAPIError, and reverts the recent strict type checks inEndpointdivision operators. Version bumped to0.2.0b1.Refactors
SyncDataResource/AsyncDataResourceas the main entry;Faceit/AsyncFaceitremain but are deprecated.faceit.resources.aggregator.gather_pagesreturnsItemPage[...]for models andlistfor raw. RemovedMaxPages; usepages(...).ItemPageaddsget_first/get_last/get_random(withfirst/last/randomkept as aliases).tenacity, now retries onAPIError(and raisesAPIErrorfor server errors), improved SSL error handling, and masked API key now shows prefix/suffix.GameInfo.skill_levelnow auto-converts toSkillLevel;SkillLevelsupports equality and ordering comparisons.Taskfile.yml; README replaced with a temporary placeholder. Added Python 3.14 classifier.Migration
Faceit.data(...)withSyncDataResource(...)andAsyncFaceit.data(...)withAsyncDataResource(...). Both accept an API key or a prebuilt client.MaxPages(...)withpages(...).get_first/get_last/get_randomonItemPage(old names still work).GameInfo.skill_levelas an int, cast it:int(info.level).client.api_keyin tests, expect a masked prefix/suffix instead of[REDACTED].httpx.HTTPStatusError, catchAPIErrorinstead.Written for commit 7946a56. Summary will update on new commits.