feat: возможность для пользователя передавать тип загружаемого файла как строку#119
feat: возможность для пользователя передавать тип загружаемого файла как строку#119someqst wants to merge 4 commits intolove-apples:mainfrom
Conversation
e042e8a to
ad07ed4
Compare
There was a problem hiding this comment.
Pull request overview
PR добавляет возможность передавать тип загружаемого файла как строку (в дополнение к UploadType) и выносит определение типа по содержимому в общий хелпер, чтобы убрать дублирование.
Changes:
- Расширен API
InputMediaдля приемаtypeкакUploadType | str. - Вынесена логика определения типа файла по содержимому в функцию
detect_file_type. - Унифицирована валидация
typeс выдачей понятной ошибки при неверном значении.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Определяет тип файла на основе его содержимого (MIME-типа). | ||
|
|
||
| Args: | ||
| data (bytes): Буфер с содержимым файла. |
There was a problem hiding this comment.
В detect_file_type докстринг описывает аргументы, но не документирует возвращаемое значение (и возможные исключения). В этом репозитории обычно указываются секции Returns: (и часто Raises:) — стоит добавить их, чтобы поведение функции было однозначно описано.
| data (bytes): Буфер с содержимым файла. | |
| data (bytes): Буфер с содержимым файла. | |
| Returns: | |
| UploadType: Определенный тип файла. Если MIME-тип не удалось | |
| определить или при определении произошла ошибка, возвращается | |
| ``UploadType.FILE``. |
| if type is not None: | ||
| try: | ||
| self.type = UploadType(type) | ||
| except ValueError as e: | ||
| allowed = ", ".join(item.value for item in UploadType) |
There was a problem hiding this comment.
Сигнатура InputMediaBuffer.__init__ по-прежнему аннотирует type как UploadType | None, но ниже вы явно поддерживаете строки через UploadType(type). Это делает новый публичный API несогласованным (и ухудшает опыт статической типизации). Обновите аннотацию параметра до UploadType | str | None (и синхронизируйте описание аргумента в докстринге).
| if type is not None: | ||
| try: | ||
| self.type = UploadType(type) | ||
| except ValueError as e: | ||
| allowed = ", ".join(item.value for item in UploadType) | ||
| raise ValueError( | ||
| f"Неверный тип загружаемого файла: {type!r}. Ожидается: {allowed}" # noqa: E501 | ||
| ) from e |
There was a problem hiding this comment.
Логика приведения/валидации type (через UploadType(type) + формирование списка allowed) продублирована в InputMedia и InputMediaBuffer. Лучше вынести её в общий хелпер (например, модульную функцию), чтобы в будущем не разъехались формат ошибки/правила валидации.
| def __init__(self, path: str, type: UploadType | str | None = None): | ||
| """ | ||
| Инициализирует объект медиафайла. | ||
|
|
||
| Args: | ||
| path (str): Путь к файлу. | ||
| type (UploadType, optional): Тип файла. Если не указан, | ||
| type (UploadType, str, optional): Тип файла. Если не указан, | ||
| определяется автоматически. | ||
| """ | ||
|
|
||
| self.path = path | ||
| self.type = type or self.__detect_file_type(path) | ||
|
|
||
| def __detect_file_type(self, path: str) -> UploadType: | ||
| """ | ||
| Определяет тип файла на основе его содержимого (MIME-типа). | ||
|
|
||
| Args: | ||
| path (str): Путь к файлу. | ||
|
|
||
| Returns: | ||
| UploadType: Тип файла (VIDEO, IMAGE, AUDIO или FILE). | ||
| """ | ||
| if type is not None: | ||
| try: | ||
| self.type = UploadType(type) | ||
| except ValueError as e: | ||
| allowed = ", ".join(item.value for item in UploadType) | ||
| raise ValueError( | ||
| f"Неверный тип загружаемого файла: {type!r}. Ожидается: {allowed}" # noqa: E501 | ||
| ) from e | ||
| else: | ||
| self.type = detect_file_type(InputMedia._read_file_sample(path)) |
There was a problem hiding this comment.
Добавлена новая функциональность: принимать type как строку и выбрасывать ValueError с перечислением допустимых значений. В тестах сейчас нет проверок этого сценария (особенно: строка валидная → корректный UploadType, строка невалидная → ожидаемое сообщение ошибки). Стоит добавить/расширить тесты для InputMedia и InputMediaBuffer, чтобы зафиксировать контракт.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Хорошая работа. Рефакторинг детекции типа в detect_file_type — правильное решение, убирает дублирование между InputMedia и InputMediaBuffer. Логика валидации с понятным сообщением об ошибке тоже на месте.
Тесты покрывают основные сценарии, включая проверку что при явно переданном типе автодетект не вызывается — именно то, что нужно.
Судя по боту coverage - где-то пропущено покрытие.
По комментариям Copilot хорошо бы пробежаться внимательно. Если не актуально - напишите, позакрываю.
Косметический момент: в docstring InputMediaBuffer.init порядок типов записан как type (UploadType, optional, str) — немного странно, лучше UploadType | str | None как в сигнатуре. Не блокирует.
|
@Olegt0rr |
…как строку вынесена валидация типа медиа обновлен docstring detect_file_type исправлены аннотации добавлены тесты на проверку InputMedia и InputMediaBuffer исправлена ошибка ruff fix: исправлены замечания ruff fix: исправлены docstring feat: дополнены тесты mime_type=None fix: исправлена логика validate_uploading_type исправлены замечания ruff fix: formatting
16f82ef to
413abae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| matches = puremagic.magic_string(data) | ||
| if matches: | ||
| mime_type = matches[0].mime_type |
There was a problem hiding this comment.
puremagic.magic_string() results are treated as tuples elsewhere in the codebase (e.g. maxapi/connection/base.py:265-268 uses matches[0][1] for MIME). Here matches[0].mime_type will raise AttributeError when matches[0] is a tuple. Please extract MIME consistently (e.g. tuple indexing) or normalize the result before accessing it.
| mime_type = matches[0].mime_type | |
| match = matches[0] | |
| if isinstance(match, (tuple, list)) and len(match) > 1: | |
| mime_type = match[1] | |
| else: | |
| mime_type = getattr(match, "mime_type", None) |
There was a problem hiding this comment.
Здесь matches - это list из namedtuple. Луче обратиться по атрибуту. Исправил поведение в maxapi/connection/base.py, там обращение в принципе было не к mime_type по индексу элемента в кортеже.
| Attributes: | ||
| path (str): Путь к файлу. | ||
| type (UploadType): Тип файла, определенный на основе содержимого | ||
| type (UploadType | str | None): Тип файла, определенный на основе содержимого | ||
| (MIME-типа). | ||
| """ | ||
| """ # noqa: E501 |
There was a problem hiding this comment.
InputMedia.type is always set to an UploadType instance (either via validate_uploading_type() or detect_file_type()), but the docstring/types currently describe the attribute as UploadType | str | None. Consider keeping the constructor parameter as UploadType | str | None but tightening the stored attribute annotation/docstring to UploadType to avoid misleading users and type-checkers.
| Attributes: | ||
| buffer (bytes): Буфер с содержимым файла. | ||
| type (UploadType): Тип файла, определенный по содержимому. | ||
| type (UploadType | str | None): Тип файла, определенный по содержимому. | ||
| """ |
There was a problem hiding this comment.
InputMediaBuffer.type is always set to an UploadType instance after initialization, but the docstring/types currently describe the attribute as UploadType | str | None. Consider documenting/annotating the attribute as UploadType (while keeping the constructor parameter as UploadType | str | None) so consumers and type-checkers don't assume str/None are possible post-init.
| @pytest.mark.asyncio | ||
| async def test_default_upload_type_input_media_buffer(self, tmp_path): | ||
| """ | ||
| Если mimetype не определился (None), | ||
| для файла должен вернуться тип UploadType.FILE | ||
| """ | ||
| media = InputMediaBuffer( | ||
| buffer=b"fake-bytes", | ||
| filename="sample.bin", | ||
| ) | ||
|
|
||
| assert media.type == UploadType.FILE | ||
|
|
There was a problem hiding this comment.
These tests are marked async/@pytest.mark.asyncio but don't await anything. Making them synchronous (def ...) and dropping the asyncio marker will reduce overhead and avoid implying async behavior where there is none.
| @pytest.mark.asyncio | ||
| async def test_default_upload_type_input_media(self, tmp_path): | ||
| """ | ||
| Если mimetype не определился (None), | ||
| для файла должен вернуться тип UploadType.FILE | ||
| """ | ||
| test_file = tmp_path / "sample.bin" | ||
| test_file.write_bytes(b"fake-data") | ||
|
|
||
| media = InputMedia(path=test_file) | ||
|
|
||
| assert media.type == UploadType.FILE |
There was a problem hiding this comment.
This test is marked async/@pytest.mark.asyncio but doesn't await anything. Converting it to a normal synchronous test (def ...) and dropping the asyncio marker will make intent clearer and avoid unnecessary event-loop setup.
| Args: | ||
| path (str): Путь к файлу. | ||
| type (UploadType, optional): Тип файла. Если не указан, | ||
| type (UploadType | str |None): Тип файла. Если не указан, |
There was a problem hiding this comment.
Docstring type annotation has a formatting typo: UploadType | str |None is missing a space before None (| None). This is minor but it makes the generated docs/type hints harder to read.
| type (UploadType | str |None): Тип файла. Если не указан, | |
| type (UploadType | str | None): Тип файла. Если не указан, |
|
@Olegt0rr |
Столкнулся с проблемой, что тип загружаемого файла нельзя передать строкой.
Думаю, стоит позволить пользователю передавать строку.
Также сделал небольшой рефакторинг функционала определения типа файла на основе содержимого во избежание дублирования кода.