Dev/1.2.3 #33
Dev/1.2.3 #33
Conversation
… для поддержки новых типов вложений
📝 WalkthroughWalkthroughThis PR adds contact management capabilities to PyMax, introduces profile photo upload functionality via an extended change_profile method, and implements contact synchronization from raw payload data. The changes also include type system enhancements for contact attachments, updates to example code demonstrating new event handlers and filters, and a version bump to 1.2.3. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client<br/>(SelfMixin)
participant UploadSvc as Upload<br/>Service
participant ProfileSvc as Profile<br/>Service
Client->>Client: change_profile(photo=Photo(...))
alt photo provided
Client->>UploadSvc: PHOTO_UPLOAD request<br/>(profile=True)
activate UploadSvc
UploadSvc-->>Client: upload_url
deactivate UploadSvc
Note over Client,UploadSvc: _upload_profile_photo flow
Client->>UploadSvc: multipart form POST<br/>(photo to upload_url)
activate UploadSvc
UploadSvc-->>Client: photo_token
deactivate UploadSvc
Client->>ProfileSvc: CHANGE_PROFILE<br/>(photo_token, names...)
activate ProfileSvc
ProfileSvc-->>Client: profile contact data
deactivate ProfileSvc
Client->>Client: self.me = Me.from_dict(...)
else no photo
Client->>ProfileSvc: CHANGE_PROFILE<br/>(names only)
activate ProfileSvc
ProfileSvc-->>Client: profile contact data
deactivate ProfileSvc
Client->>Client: self.me = Me.from_dict(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/pymax/files.py (1)
48-54: Consider aligning initialization pattern with Video and File classes.The file_name initialization works correctly but differs slightly from Video (lines 86-95) and File (lines 103-113) classes, which initialize file_name to an empty string first and then validate it. While not critical, aligning the pattern would improve consistency.
🔎 Proposed refactor for consistency
def __init__(self, url: str | None = None, path: str | None = None) -> None: + self.file_name: str = "" if path: self.file_name = Path(path).name elif url: self.file_name = Path(url).name super().__init__(url, path)src/pymax/mixins/socket.py (1)
608-614: LGTM!The contacts parsing implementation follows the established pattern for chats, dialogs, and channels. The per-entry error handling ensures that a single malformed contact doesn't break the entire sync process.
Optional: Include contacts count in sync completion log
Consider updating the sync completion log message (lines 618-623) to include the contacts count for better observability:
self.logger.info( - "Sync completed: dialogs=%d chats=%d channels=%d", + "Sync completed: dialogs=%d chats=%d channels=%d contacts=%d", len(self.dialogs), len(self.chats), len(self.channels), + len(self.contacts), )src/pymax/mixins/websocket.py (1)
469-475: LGTM!The contacts parsing implementation is consistent with both the chat parsing in this file and the socket.py implementation. The per-entry error handling provides good resilience.
Optional: Include contacts count in sync completion log
Consider updating the sync completion log message (lines 480-485) to include the contacts count for consistency with the suggestion in socket.py:
self.logger.info( - "Sync completed: dialogs=%d chats=%d channels=%d", + "Sync completed: dialogs=%d chats=%d channels=%d contacts=%d", len(self.dialogs), len(self.chats), len(self.channels), + len(self.contacts), )src/pymax/mixins/self.py (2)
1-4: Unused import:urllib.parse.Line 1 imports
urllib.parsebut is not used directly. Line 4 importsparse_qsandurlparsefromurllib.parse, which are the actual functions used in the code.🔎 Suggested fix
-import urllib.parse from http import HTTPStatus from typing import Any from urllib.parse import parse_qs, urlparse
87-108: Consider consolidating duplicated payload construction.The payload construction logic is nearly identical in both branches, differing only by the inclusion of
photo_token. This can be simplified.🔎 Suggested refactor
+ photo_token: str | None = None if photo: upload_url = await self._request_photo_upload_url() photo_token = await self._upload_profile_photo(upload_url, photo) - payload = ChangeProfilePayload( - first_name=first_name, - last_name=last_name, - description=description, - photo_token=photo_token, - ).model_dump( - by_alias=True, - exclude_none=True, - ) - else: - payload = ChangeProfilePayload( - first_name=first_name, - last_name=last_name, - description=description, - ).model_dump( - by_alias=True, - exclude_none=True, - ) + payload = ChangeProfilePayload( + first_name=first_name, + last_name=last_name, + description=description, + photo_token=photo_token, + ).model_dump( + by_alias=True, + exclude_none=True, + )src/pymax/types.py (1)
50-50: Minor: Unconventional type annotation order.
None | strworks butstr | Noneis the more conventional ordering in Python. This is a minor style inconsistency.🔎 Suggested fix
- first_name: None | str, + first_name: str | None,Also applies to: 93-93
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
examples/example.pyexamples/flt_test.pyexamples/large_file_upload.pyexamples/reg.pyexamples/test.pypyproject.tomlsrc/pymax/core.pysrc/pymax/files.pysrc/pymax/interfaces.pysrc/pymax/mixins/group.pysrc/pymax/mixins/self.pysrc/pymax/mixins/socket.pysrc/pymax/mixins/websocket.pysrc/pymax/payloads.pysrc/pymax/static/enum.pysrc/pymax/types.py
💤 Files with no reviewable changes (4)
- examples/test.py
- examples/flt_test.py
- examples/reg.py
- examples/large_file_upload.py
🧰 Additional context used
🧬 Code graph analysis (8)
src/pymax/interfaces.py (1)
src/pymax/types.py (1)
User(952-1007)
src/pymax/core.py (1)
src/pymax/types.py (1)
User(952-1007)
src/pymax/mixins/socket.py (1)
src/pymax/types.py (17)
User(952-1007)from_dict(34-35)from_dict(72-78)from_dict(146-156)from_dict(190-205)from_dict(246-260)from_dict(284-292)from_dict(323-332)from_dict(367-376)from_dict(415-426)from_dict(452-459)from_dict(483-487)from_dict(502-511)from_dict(532-540)from_dict(558-559)from_dict(577-582)from_dict(601-602)
src/pymax/mixins/group.py (3)
src/pymax/interfaces.py (2)
_send_and_wait(97-104)_get_chat(107-108)src/pymax/mixins/websocket.py (2)
_send_and_wait(328-360)_get_chat(496-500)src/pymax/payloads.py (1)
GetChatInfoPayload(335-336)
examples/example.py (4)
src/pymax/files.py (3)
File(102-117)Photo(38-82)Video(85-99)src/pymax/filters.py (2)
chat(135-136)text(139-140)src/pymax/payloads.py (1)
UserAgentPayload(42-53)src/pymax/mixins/handler.py (1)
on_raw_receive(129-142)
src/pymax/mixins/self.py (5)
src/pymax/files.py (5)
Photo(38-82)read(23-35)read(81-82)read(98-99)read(116-117)src/pymax/payloads.py (2)
UploadPayload(98-100)ChangeProfilePayload(168-173)src/pymax/types.py (17)
Me(514-548)from_dict(34-35)from_dict(72-78)from_dict(146-156)from_dict(190-205)from_dict(246-260)from_dict(284-292)from_dict(323-332)from_dict(367-376)from_dict(415-426)from_dict(452-459)from_dict(483-487)from_dict(502-511)from_dict(532-540)from_dict(558-559)from_dict(577-582)from_dict(601-602)src/pymax/mixins/websocket.py (1)
_send_and_wait(328-360)src/pymax/mixins/utils.py (2)
MixinsUtils(6-27)handle_error(8-27)
src/pymax/mixins/websocket.py (1)
src/pymax/types.py (17)
User(952-1007)from_dict(34-35)from_dict(72-78)from_dict(146-156)from_dict(190-205)from_dict(246-260)from_dict(284-292)from_dict(323-332)from_dict(367-376)from_dict(415-426)from_dict(452-459)from_dict(483-487)from_dict(502-511)from_dict(532-540)from_dict(558-559)from_dict(577-582)from_dict(601-602)
src/pymax/types.py (1)
src/pymax/static/enum.py (2)
AttachType(192-199)FormattingType(202-206)
🪛 Ruff (0.14.10)
src/pymax/mixins/group.py
304-304: Avoid specifying long messages outside the exception class
(TRY003)
src/pymax/mixins/self.py
57-59: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (19)
src/pymax/static/enum.py (1)
199-199: LGTM!The addition of the CONTACT member to the AttachType enum is consistent with the existing enum structure and supports the new contact attachment functionality introduced in this PR.
pyproject.toml (1)
3-3: LGTM!Version bump to 1.2.3 is appropriate for the new features and bug fixes introduced in this PR.
src/pymax/core.py (1)
119-119: LGTM!The addition of the contacts collection follows the established pattern for chats, dialogs, and channels. The initialization and type annotation are correct.
src/pymax/interfaces.py (1)
48-48: LGTM!The contacts attribute addition to ClientProtocol correctly extends the interface to support the new contact management feature.
src/pymax/payloads.py (2)
100-100: LGTM!The profile field addition enables profile-specific photo upload workflows, complementing the existing count field.
172-173: LGTM with TODO noted.The photo_token and avatar_type fields appropriately support profile photo upload functionality. The TODO comment correctly identifies that avatar_type should be extracted to an enum for better type safety.
src/pymax/mixins/socket.py (1)
31-31: LGTM!The User import is correctly added to support contact parsing in the sync method.
src/pymax/mixins/websocket.py (1)
30-30: LGTM!The User import is correctly added to support contact parsing in the sync method.
src/pymax/mixins/group.py (2)
98-98: LGTM!These are minor formatting changes that consolidate the
_send_and_waitcalls into single-line expressions. The functionality remains unchanged.Also applies to: 156-156
355-358: LGTM!Minor formatting adjustments with no functional changes.
examples/example.py (3)
8-10: LGTM!The new imports align with the expanded functionality demonstrated in this example file.
33-36: LGTM!Good example of the new
on_raw_receiveevent handler for debugging/logging raw server data.
50-57: LGTM!Good examples of the
on_chat_updatehandler and the newFiltersusage foron_message. The filterFilters.chat(0) & Filters.text("hello")clearly demonstrates combining multiple filters.src/pymax/mixins/self.py (2)
26-37: LGTM!The
_request_photo_upload_urlmethod correctly requests an upload URL using thePHOTO_UPLOADopcode withprofile=True, handles errors viaMixinsUtils, and returns the URL from the response.
115-116: LGTM!Good addition to update
self.mewith the returned profile data after a successful profile change.src/pymax/types.py (4)
220-220: LGTM!Good change to make
lottie_urloptional and use.get("lottieUrl")for safe access. This handles stickers that may not have a Lottie animation URL.Also applies to: 249-249
633-661: LGTM!The new
ContactAttachclass is well-structured and follows the established patterns for other attach types. It correctly:
- Defines all required fields in
__init__- Sets
self.type = AttachType.CONTACT- Implements
from_dict,__repr__, and__str__consistently with other attach classes
677-729: LGTM!The
Messageclass is correctly updated to includeContactAttachin the attaches union type, andfrom_dictproperly handles the newAttachType.CONTACTcase by instantiatingContactAttach.from_dict(a).
814-814: LGTM!Minor formatting improvements that consolidate expressions into single lines without changing functionality.
Also applies to: 899-899, 902-902
| @client.on_reaction_change | ||
| async def handle_reaction_change( | ||
| message_id: str, chat_id: int, reaction_info: ReactionInfo | ||
| ) -> None: | ||
| print( | ||
| f"Reaction changed on message {message_id} in chat {chat_id}: " | ||
| f"Total count: {reaction_info.total_count}, " | ||
| f"Your reaction: {reaction_info.your_reaction}, " | ||
| f"Counters: {reaction_info.counters[0].reaction}={reaction_info.counters[0].count}" | ||
| ) |
There was a problem hiding this comment.
Potential IndexError when accessing counters[0].
Line 46 accesses reaction_info.counters[0] without verifying that the counters list is non-empty. If a reaction change event arrives with an empty counters list, this will raise an IndexError.
Since this is example code, consider adding a guard or a comment noting this assumption.
🔎 Suggested fix
@client.on_reaction_change
async def handle_reaction_change(
message_id: str, chat_id: int, reaction_info: ReactionInfo
) -> None:
+ counters_info = (
+ f"{reaction_info.counters[0].reaction}={reaction_info.counters[0].count}"
+ if reaction_info.counters
+ else "none"
+ )
print(
f"Reaction changed on message {message_id} in chat {chat_id}: "
f"Total count: {reaction_info.total_count}, "
f"Your reaction: {reaction_info.your_reaction}, "
- f"Counters: {reaction_info.counters[0].reaction}={reaction_info.counters[0].count}"
+ f"Counters: {counters_info}"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @client.on_reaction_change | |
| async def handle_reaction_change( | |
| message_id: str, chat_id: int, reaction_info: ReactionInfo | |
| ) -> None: | |
| print( | |
| f"Reaction changed on message {message_id} in chat {chat_id}: " | |
| f"Total count: {reaction_info.total_count}, " | |
| f"Your reaction: {reaction_info.your_reaction}, " | |
| f"Counters: {reaction_info.counters[0].reaction}={reaction_info.counters[0].count}" | |
| ) | |
| @client.on_reaction_change | |
| async def handle_reaction_change( | |
| message_id: str, chat_id: int, reaction_info: ReactionInfo | |
| ) -> None: | |
| counters_info = ( | |
| f"{reaction_info.counters[0].reaction}={reaction_info.counters[0].count}" | |
| if reaction_info.counters | |
| else "none" | |
| ) | |
| print( | |
| f"Reaction changed on message {message_id} in chat {chat_id}: " | |
| f"Total count: {reaction_info.total_count}, " | |
| f"Your reaction: {reaction_info.your_reaction}, " | |
| f"Counters: {counters_info}" | |
| ) |
🤖 Prompt for AI Agents
In examples/example.py around lines 38 to 47, the example prints
reaction_info.counters[0] without checking that reaction_info.counters is
non-empty which can raise IndexError; update the handler to guard against an
empty counters list (e.g., check len(reaction_info.counters) > 0 before
accessing index 0 or select a safe default when empty) or add a brief comment
explaining the assumption so the example is safe and clear.
| async def resolve_group_by_link(self, link: str) -> Chat | None: | ||
| """ | ||
| Разрешает группу по ссылке | ||
| Args: | ||
| link (str): Ссылка на группу. | ||
| Returns: | ||
| Chat | None: Объект чата группы или None, если не найдено. | ||
| """ | ||
| proceed_link = self._process_chat_join_link(link) | ||
| if proceed_link is None: | ||
| raise ValueError("Invalid group link") | ||
|
|
||
| data = await self._send_and_wait( | ||
| opcode=Opcode.LINK_INFO, | ||
| payload={ | ||
| "link": proceed_link, | ||
| }, | ||
| ) | ||
|
|
||
| if data.get("payload", {}).get("error"): | ||
| MixinsUtils.handle_error(data) | ||
|
|
||
| chat = Chat.from_dict(data["payload"].get("chat", {})) | ||
| return chat |
There was a problem hiding this comment.
Return type inconsistency: method always returns Chat, never None.
The return type annotation is Chat | None, but the method always returns a Chat object. If the server response doesn't include a "chat" key, Chat.from_dict({}) is called with an empty dict, which will create a Chat with default/zero values rather than returning None.
Consider either:
- Changing the return type to just
Chatif an empty/default Chat is acceptable, or - Adding a check to return
Nonewhen the chat data is missing/empty.
🔎 Option 2: Return None when chat is not found
- chat = Chat.from_dict(data["payload"].get("chat", {}))
- return chat
+ chat_data = data["payload"].get("chat")
+ if not chat_data:
+ return None
+ return Chat.from_dict(chat_data)🧰 Tools
🪛 Ruff (0.14.10)
304-304: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/pymax/mixins/group.py around lines 292 to 317 the method is annotated to
return Chat | None but always calls Chat.from_dict({}) when the server payload
lacks "chat", producing a default Chat instead of None; change the logic to
inspect data.get("payload", {}).get("chat") and if it's missing or empty return
None, otherwise call Chat.from_dict(...) and return the Chat; update any type
hints or tests if needed to reflect the new None-return behavior.
| async def _upload_profile_photo(self, upload_url: str, photo: Photo) -> str: | ||
| self.logger.info("Uploading profile photo") | ||
|
|
||
| parsed_url = urlparse(upload_url) | ||
| photo_id = parse_qs(parsed_url.query)["photoIds"][0] | ||
|
|
||
| form = aiohttp.FormData() | ||
| form.add_field( | ||
| "file", | ||
| await photo.read(), | ||
| filename=photo.file_name, | ||
| ) | ||
|
|
||
| async with ( | ||
| aiohttp.ClientSession() as session, | ||
| session.post(upload_url, data=form) as response, | ||
| ): | ||
| if response.status != HTTPStatus.OK: | ||
| raise Error( | ||
| "Failed to upload profile photo.", message="UploadError", title="Upload Error" | ||
| ) | ||
|
|
||
| self.logger.info("Upload successful") | ||
| data = await response.json() | ||
| return data["photos"][photo_id][ | ||
| "token" | ||
| ] # TODO: сделать нормальную типизацию и чекнинг ответа |
There was a problem hiding this comment.
Potential KeyError when parsing photoIds from URL.
Line 43 assumes the photoIds query parameter exists in the upload URL. If the server returns a URL without this parameter, a KeyError will be raised.
Consider adding defensive handling:
🔎 Suggested fix
parsed_url = urlparse(upload_url)
- photo_id = parse_qs(parsed_url.query)["photoIds"][0]
+ query_params = parse_qs(parsed_url.query)
+ photo_ids = query_params.get("photoIds")
+ if not photo_ids:
+ raise Error(
+ "Missing photoIds in upload URL", message="UploadError", title="Upload Error"
+ )
+ photo_id = photo_ids[0]🧰 Tools
🪛 Ruff (0.14.10)
57-59: Avoid specifying long messages outside the exception class
(TRY003)
Описание
Кратко, что делает этот PR. Например, добавляет новый метод, исправляет баг, улучшает документацию.
Новые методы:
Измененные методы:
Новые типы:
Измененные типы:
Новые парамерты
Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.