-
Notifications
You must be signed in to change notification settings - Fork 9
Обращение к API бэкенда в команде ls -r #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughВнесены изменения в функцию получения списка доступных к установке версий: теперь сначала выполняется попытка получить версии через новый API-эндпоинт ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ВерсииOneScript
participant API
participant HTML
Client->>ВерсииOneScript: ПолучитьСписокДоступныхКУстановкеВерсий()
ВерсииOneScript->>API: ПолучитьВерсииПоAPI()
API-->>ВерсииOneScript: Ответ (успех/ошибка)
alt API ответ успешен
ВерсииOneScript-->>Client: Список версий из API
else API ошибка
ВерсииOneScript->>HTML: ПолучитьВерсииРазборомHTML()
HTML-->>ВерсииOneScript: Список версий из HTML
ВерсииOneScript-->>Client: Список версий из HTML
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/Классы/ВерсииOneScript.os (3)
220-227
: Явно укажите использование HTTPS и уберите «пустые» аргументыВ конструкторе
HTTPСоединение
передаются пять позиционных аргументов, из которых четыре — пустые, что затрудняет понимание, какой именно параметр отвечает за SSL, порт и т.д.
Предлагаю явно проставить флагИстина
для использования HTTPS и заменить пустые значения параметрами-заглушками (Неопределено
), либо воспользоваться именованными параметрами (если поддерживается), чтобы читаемость не страдала.Соединение = Новый HTTPСоединение( - ПараметрыOVM.АдресСайтаОСкрипт(), - , - , - , - , - Таймаут + ПараметрыOVM.АдресСайтаОСкрипт(), // Хост + , // Порт + , // БазовыйПуть + Истина, // ИспользоватьSSL + , // ЗаголовкиПоУмолчанию + Таймаут );
339-343
: Возможны дубликаты алиасов из-за отсутствия проверки уникальностиПоле
presentation
в JSON может совпадать с уже добавленным алиасом (например,latest
). При повторном добавлении строка просто продублируется, что усложнит дальнейшую обработку. Рассмотрите вариант использоватьНайти()
передДобавить()
или перенести логику вОбеспечитьСтрокуВерсииПоАлиасу
.
381-384
: Жёстко «захардкоженная» версия 1.0.9 — технический долгФильтр версии
1.0.9
реализован как временное решение (TODO
). Рекомендуется:
- Переместить исключения версий в конфигурацию или константу.
- Добавить ссылку на задачу/issue в комментарий (уже сделано) и установить крайний срок её удаления.
Это уменьшит вероятность того, что хак останется навсегда.
src/core/Классы/ВерсииOneScript.os
Outdated
Запрос = Новый HTTPЗапрос("api/archive"); | ||
Ответ = Соединение.Получить(Запрос); | ||
HTTP_OK = 200; | ||
Если Ответ.КодСостояния <> HTTP_OK Тогда | ||
Возврат Неопределено; | ||
КонецЕсли; | ||
|
||
ЧтениеJSON = Новый ЧтениеJSON(); | ||
ЧтениеJSON.УстановитьСтроку(Ответ.ПолучитьТелоКакСтроку()); | ||
|
||
ДоступныеВерсии = Новый ТаблицаЗначений; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нет обработки ошибок разбора JSON – возможны необработанные исключения
Если API вернёт невалидный JSON, то ПрочитатьJSON
выбросит исключение и команда рухнет. Лучше перехватить ошибку и вернуть Неопределено
, чтобы сработал резервный сценарий парсинга HTML.
- ЧтениеJSON = Новый ЧтениеJSON();
- ЧтениеJSON.УстановитьСтроку(Ответ.ПолучитьТелоКакСтроку());
+ Попытка
+ ЧтениеJSON = Новый ЧтениеJSON();
+ ЧтениеJSON.УстановитьСтроку(Ответ.ПолучитьТелоКакСтроку());
+ Исключение
+ // Логируем, но не падаем – будет попытка резервного метода
+ Лог.Ошибка("Не удалось разобрать JSON об архиве версий: %1", ОписаниеОшибки());
+ Возврат Неопределено;
+ КонецПопытки;
📝 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.
Запрос = Новый HTTPЗапрос("api/archive"); | |
Ответ = Соединение.Получить(Запрос); | |
HTTP_OK = 200; | |
Если Ответ.КодСостояния <> HTTP_OK Тогда | |
Возврат Неопределено; | |
КонецЕсли; | |
ЧтениеJSON = Новый ЧтениеJSON(); | |
ЧтениеJSON.УстановитьСтроку(Ответ.ПолучитьТелоКакСтроку()); | |
ДоступныеВерсии = Новый ТаблицаЗначений; | |
Запрос = Новый HTTPЗапрос("api/archive"); | |
Ответ = Соединение.Получить(Запрос); | |
HTTP_OK = 200; | |
Если Ответ.КодСостояния <> HTTP_OK Тогда | |
Возврат Неопределено; | |
КонецЕсли; | |
Попытка | |
ЧтениеJSON = Новый ЧтениеJSON(); | |
ЧтениеJSON.УстановитьСтроку(Ответ.ПолучитьТелоКакСтроку()); | |
Исключение | |
// Логируем, но не падаем – будет попытка резервного метода | |
Лог.Ошибка("Не удалось разобрать JSON об архиве версий: %1", ОписаниеОшибки()); | |
Возврат Неопределено; | |
КонецПопытки; | |
ДоступныеВерсии = Новый ТаблицаЗначений; |
@@ -365,6 +405,8 @@ | |||
СтрокаВерсии.Алиас = Алиас; | |||
КонецЕсли; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не совсем понятно, что надо добавлять. И можно ли на это заложиться уже сейчас, чтобы не делать две версии овм под новую структуру сайта?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сейчас в качестве урла для метаверсий в выхлопе команды ls -r
пишется просто /downloads, а в новой версии будет работать линк /downloads/lts и /downoads/preview.
Но если сейчас это написать в ovm то эти линки приведут на 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что, наверное, не страшно, т.к. по этим линкам все равно вряд ли кто-то ходит...
Добавлено получение удаленных версий из API oscript.io
Summary by CodeRabbit