Рефакторинг сериализации и десериализации объектов #28
Conversation
📝 WalkthroughWalkthroughReplaces table-based "Расшифровка" with array-based "Запуски"; adds JSON serializers/DTOs and custom-jason annotations; removes XML exporter and xml-parser; changes CLI flag Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (--settings)
participant Runner as КомандаЗапуска
participant Serializer as СериализаторНастроекБенчмарков
participant Loader as КоллекцияДескрипторовБенчмарков
participant Launcher as ЗапускательБенчмарков
participant Exporters as Экспортеры (Md/Json/Html)
CLI->>Runner: parse --settings path
Runner->>Serializer: ПрочитатьИзJson(path)
Serializer-->>Runner: НастройкиБенчмарковДто
Runner->>Loader: ПодключитьТипы(Настройки)
Loader-->>Runner: ДескрипторыБенчмарков
Runner->>Launcher: ЗапуститьПоТипам(Дескрипторы, Конфигурация)
Launcher->>Launcher: выполнить запуски -> сформировать РезультатыБенчмарков(Запуски)
Launcher->>Exporters: ЭкспортироватьРезультаты(РезультатыБенчмарков, ПолноеИмяФайла)
Exporters-->>Runner: files written
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/BenchmarkOneScript/core/Модули/ИзвлекательКонфигурацииБенчмарков.os (1)
37-44: Bug: wrong config variable passed; can cause NPE and lost settings.
Конфигурация(input, possibly Неопределено) is passed into readers instead ofКонфигурацияБенчмарков. This can throw at runtime and/or not apply settings to the returned object.Apply this fix:
- ПрочитатьАннотацииКолонок(СвойстваКонструктора, Конфигурация); - ПрочитатьАннотациюКаталогАртефактов(СвойстваКонструктора, Конфигурация); - ПрочитатьАннотациюЭкспортMarkdown(СвойстваКонструктора, Конфигурация); - ПрочитатьАннотациюЭкспортJson(СвойстваКонструктора, Конфигурация); - ПрочитатьАннотациюЭкспортXml(СвойстваКонструктора, Конфигурация); - ПрочитатьАннотациюЭкспортHtml(СвойстваКонструктора, Конфигурация); - ПрочитатьАннотациюИсполняющаяСреда(СвойстваКонструктора, Конфигурация); + ПрочитатьАннотацииКолонок(СвойстваКонструктора, КонфигурацияБенчмарков); + ПрочитатьАннотациюКаталогАртефактов(СвойстваКонструктора, КонфигурацияБенчмарков); + ПрочитатьАннотациюЭкспортMarkdown(СвойстваКонструктора, КонфигурацияБенчмарков); + ПрочитатьАннотациюЭкспортJson(СвойстваКонструктора, КонфигурацияБенчмарков); + ПрочитатьАннотациюЭкспортXml(СвойстваКонструктора, КонфигурацияБенчмарков); + ПрочитатьАннотациюЭкспортHtml(СвойстваКонструктора, КонфигурацияБенчмарков); + ПрочитатьАннотациюИсполняющаяСреда(СвойстваКонструктора, КонфигурацияБенчмарков);- ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПередВсеми, РефлекторОбъекта, Конфигурация); - ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПослеВсех, РефлекторОбъекта, Конфигурация); - ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПередКаждым, РефлекторОбъекта, Конфигурация); - ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПослеКаждого, РефлекторОбъекта, Конфигурация); + ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПередВсеми, РефлекторОбъекта, КонфигурацияБенчмарков); + ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПослеВсех, РефлекторОбъекта, КонфигурацияБенчмарков); + ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПередКаждым, РефлекторОбъекта, КонфигурацияБенчмарков); + ПрочитатьОбработчикиСобытия(СобытияБенчмарков.ПослеКаждого, РефлекторОбъекта, КонфигурацияБенчмарков);Also applies to: 51-55
src/BenchmarkOneScript/core/Классы/СериализаторРезультатовБенчмарков.os (1)
386-389: Неправильная логика проверки существования набора параметров + нестрогое сравнениеФункция ОтсутствуетНаборПараметровБенчмарка возвращает результат, зависящий от последнего набора, а не от наличия точного совпадения; возможны дубликаты. Также сравнение через «Не A = B» неочевидно.
-Функция ОтсутствуетНаборПараметровБенчмарка(ДескрипторБенчмарка, ПараметрыМетода) - - НайденНабор = Ложь; - Для Каждого НаборПараметров Из ДескрипторБенчмарка.НаборыПараметров() Цикл - НайденНабор = Истина; - МассивПараметров = НаборПараметров.ВМассив(); - Для Инд = 0 По МассивПараметров.ВГраница() Цикл - Если Не МассивПараметров[Инд].Значение = ПараметрыМетода[Инд] Тогда - НайденНабор = Ложь; - Прервать; - КонецЕсли; - КонецЦикла; - КонецЦикла; - - Возврат Не НайденНабор; -КонецФункции +Функция ОтсутствуетНаборПараметровБенчмарка(ДескрипторБенчмарка, ПараметрыМетода) + Для Каждого НаборПараметров Из ДескрипторБенчмарка.НаборыПараметров() Цикл + МассивПараметров = НаборПараметров.ВМассив(); + // Строгая проверка длины + Если МассивПараметров.Количество() <> ПараметрыМетода.Количество() Тогда + Продолжить; + КонецЕсли; + Совпадает = Истина; + Для Инд = 0 По ПараметрыМетода.ВГраница() Цикл + Если МассивПараметров[Инд].Значение <> ПараметрыМетода[Инд] Тогда + Совпадает = Ложь; + Прервать; + КонецЕсли; + КонецЦикла; + Если Совпадает Тогда + Возврат Ложь; // набор уже существует + КонецЕсли; + КонецЦикла; + Возврат Истина; // такого набора нет +КонецФункцииДополнительно: в ЗаполнитьПараметрыВСтроке (Линии 386-389) перед добавлением набора стоит убедиться, что ПараметрыМетода заполнены полностью, иначе появятся «пустые» наборы.
Also applies to: 398-407
tests/ТестыКонфигурации.os (1)
541-545: Незадействованная переменная «Процентили» — тест не проверяет 33% колонкуСоздаёте Процентили и добавляете 33, но итерируете снова ПроцентилиПоУмолчанию(), из‑за чего колонка 33% не проверяется.
-Процентили = КолонкаОтчетаБенчмарковПроцентиль.ПроцентилиПоУмолчанию(); -Процентили.Добавить(33); - -Для Каждого Процентиль Из КолонкаОтчетаБенчмарковПроцентиль.ПроцентилиПоУмолчанию() Цикл +Процентили = КолонкаОтчетаБенчмарковПроцентиль.ПроцентилиПоУмолчанию(); +Процентили.Добавить(33); +Для Каждого Процентиль Из Процентили Цикл
🧹 Nitpick comments (23)
tests/ТестыЭталонов.os (1)
1-1: Scope the LineLength suppression.Prefer limiting
// BSLLS:LineLength-offto specific long blocks and re-enabling afterwards to avoid masking unrelated issues.src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарковИзКаталога.os (1)
68-71: Log more context on failures (file and stack).Include the script path and stack to speed up troubleshooting.
Apply this diff:
- _Лог.Ошибка("Возникла ошибка при запуске бенчмарков <%1>: %2", - СтрокаРезультата.ИмяКласса, - ОписаниеОшибки()); + Ошибка = ИнформацияОбОшибке(); + _Лог.Ошибка("Ошибка при запуске бенчмарков <%1> (%2): %3", + СтрокаРезультата.ИмяКласса, + СтрокаРезультата.ПолноеИмяФайла, + ОписаниеОшибки()); + Если ТипЗнч(Ошибка) = Тип("ИнформацияОбОшибке") Тогда + _Лог.Отладка("Стек: %1", Ошибка.Текст); + КонецЕсли;src/BenchmarkOneScript/core/Классы/ЭкспортерРезультатовБенчмарковВJson.os (1)
11-11: Ensure artifacts directory exists before writing.Without a pre-created directory, write may fail.
Apply this diff:
КаталогАртефактов = РезультатыБенчмарков.Конфигурация.КаталогАртефактов(); + СоздатьКаталог(КаталогАртефактов); ПутьКФайлу = ОбъединитьПути(КаталогАртефактов, РезультатыБенчмарков.ИмяКласса + "-report." + Расширение());src/BenchmarkOneScript/dto/Классы/НастройкиБенчмарковДто.os (1)
3-6: Initialize DTO fields to avoidНеопределеноat runtime.Serializers/consumers may not handle
Неопределено.Apply this addition:
+Процедура ПриСозданииОбъекта() + Если ДескрипторыБенчмарков = Неопределено Тогда + ДескрипторыБенчмарков = Новый КоллекцияДескрипторовБенчмарков(); + КонецЕсли; + Если Конфигурация = Неопределено Тогда + Конфигурация = Новый КонфигурацияБенчмарков(); + КонецЕсли; +КонецПроцедурыOptionally align name to
...Dtofor consistency across DTOs.Also applies to: 7-9
src/BenchmarkOneScript/core/Модули/ИзвлекательКонфигурацииБенчмарков.os (2)
23-24: Minor: avoid empty argument for readability.Pass
Неопределеноexplicitly.- ТаблицаМетодов = РефлекторОбъекта.ПолучитьТаблицуМетодов(, Ложь); + ТаблицаМетодов = РефлекторОбъекта.ПолучитьТаблицуМетодов(Неопределено, Ложь);
14-21: Optional: rename local to reduce shadowing.
Конфигурация(param) vsКонфигурацияБенчмарков(local) invites mistakes (like the bug above). Rename local toРезультатКонфигурацииor similar.src/BenchmarkOneScript/core/Классы/ПараметрБенчмарка.os (1)
7-12: Добавьте базовую валидацию конструктора.Текущие значения по умолчанию ок. Рекомендую явно валидировать тип и непустоту имени (особенно при пЭтоПараметрМетода=Истина) — это раннее обнаружение ошибок конфигурации.
Процедура ПриСозданииОбъекта(пИмя = "", пЗначение = Неопределено, пЭтоПараметрМетода = Ложь) + // Бюджетная валидация без зависимостей; при желании используйте пакет validate + Если ТипЗнч(пИмя) <> Тип("Строка") Тогда + ВызватьИсключение "Имя параметра должно быть строкой"; + КонецЕсли; + Если пЭтоПараметрМетода И ПустаяСтрока(пИмя) Тогда + ВызватьИсключение "Имя параметра метода не может быть пустым"; + КонецЕсли; + Имя = пИмя; Значение = пЗначение; ЭтоПараметрМетода = пЭтоПараметрМетода;src/BenchmarkOneScript/core/Классы/КоллекцияДескрипторовБенчмарков.os (2)
18-20: Инициализация через Прочитать в конструкторе: обработайте сбой извлечения.Если извлечение бросит исключение, объект останется частично сконструирован. Перехватите/оберните ошибку с контекстом.
- Если Не ИсточникБенчмарков = Неопределено Тогда - Прочитать(ИсточникБенчмарков); - КонецЕсли; + Если ИсточникБенчмарков <> Неопределено Тогда + Попытка + Прочитать(ИсточникБенчмарков); + Исключение + ВызватьИсключение "Не удалось прочитать дескрипторы: " + ОписаниеОшибки(); + КонецПопытки; + КонецЕсли;
131-139: Сделайте Прочитать() возвращающей ЭтотОбъект для чейнинга.Это повысит удобство API и согласуется со стилем fluent.
-Процедура Прочитать(ИсточникБенчмарков) Экспорт +Функция Прочитать(ИсточникБенчмарков) Экспорт Очистить(); ИзвлекательДескрипторовБенчмарков.Извлечь(ИсточникБенчмарков, ЭтотОбъект); -КонецПроцедуры + Возврат ЭтотОбъект; +КонецФункцииТакже подтвердите, что используемый сериализатор умеет сериализовать приватные поля с &Сериализуемое (без Экспорт). Если нет — стоит пометить _ДескрипторыБенчмарков как Экспорт.
tests/ТестыБенчмарков.os (1)
761-763: Обновлённые имена API JSON-serializer — ок; добавьте защиту на IOВызовы ЗаписатьВJson/ПрочитатьИзJson корректны. Рекомендую обернуть открытие/закрытие файлов в try/finally, чтобы гарантированно закрывать дескрипторы при исключениях.
-ЗаписьJSON.ОткрытьФайл(ИмяФайла, "UTF-8", , ПараметрыЗаписиJSON); -ЗаписьJSON.ЗаписатьБезОбработки(...); -ЗаписьJSON.Закрыть(); +Попытка + ЗаписьJSON.ОткрытьФайл(ИмяФайла, "UTF-8", , ПараметрыЗаписиJSON); + ЗаписьJSON.ЗаписатьБезОбработки(...); +Исключение + // при необходимости лог +КонецПопытки; +Если ЗначениеЗаполнено(ЗаписьJSON) Тогда ЗаписьJSON.Закрыть(); КонецЕсли;src/BenchmarkOneScript/core/Классы/СериализаторРезультатовБенчмарков.os (2)
24-39: Переименование метода записи в JSON — ок; добавьте try/finallyЛогика записи не гарантирует закрытие файла при ошибке. Оберните операции в try/finally.
-ЗаписьJSON.ОткрытьФайл(ПолноеИмяФайла, "UTF-8", , ПараметрыЗаписиJSON); -ЗаписатьJSON(ЗаписьJSON, Данные); -ЗаписьJSON.Закрыть(); +Попытка + ЗаписьJSON.ОткрытьФайл(ПолноеИмяФайла, "UTF-8", , ПараметрыЗаписиJSON); + ЗаписатьJSON(ЗаписьJSON, Данные); +Исключение + // _Лог.Ошибка(...) по желанию +КонецПопытки; +Если ЗначениеЗаполнено(ЗаписьJSON) Тогда ЗаписьJSON.Закрыть(); КонецЕсли;
51-64: Переименование метода чтения из JSON — ок; закройте файл в finallyЗакрытие ЧтениеJSON не защищено на исключения. Оберните в try/finally.
-ЧтениеJSON.ОткрытьФайл(ПолноеИмяФайла, "UTF-8"); -Данные = ПрочитатьJSON(ЧтениеJSON, Ложь); -ЧтениеJSON.Закрыть(); +Попытка + ЧтениеJSON.ОткрытьФайл(ПолноеИмяФайла, "UTF-8"); + Данные = ПрочитатьJSON(ЧтениеJSON, Ложь); +Исключение + // _Лог.Ошибка(...) по желанию +КонецПопытки; +Если ЗначениеЗаполнено(ЧтениеJSON) Тогда ЧтениеJSON.Закрыть(); КонецЕсли;tests/ТестыКонфигурации.os (1)
748-792: Расширенная проверка конфигурации — ок; мелкие улучшенияПроверки полные и читаемые. Мелкие доработки:
- В сообщениях об ожиданиях стоит унифицировать «Обработчики»/«Обработчики событий».
- Для устойчивости тестов на версии исполняющей среды предпочтите поиск по значению вместо индексов (если порядок в будущем изменится).
src/BenchmarkOneScript/core/Модули/КомбинаторПараметровБенчмарка.os (2)
32-49: Нормализация входных типов — ок; добавьте валидацию содержимого массивовСейчас для Массив предполагается, что элементы — ПараметрБенчмарка. Имеет смысл явно проверять тип элементов и бросать понятную ошибку.
-ИначеЕсли ТипЭлемента = Тип("Массив") Или ТипЭлемента = Тип("ПараметрыМетодаБенчмарка") Тогда +ИначеЕсли ТипЭлемента = Тип("Массив") Или ТипЭлемента = Тип("ПараметрыМетодаБенчмарка") Тогда ... + // валидация содержимого массива (не для ПараметрыМетодаБенчмарка, т.к. он сам типизирован) + Если ТипЭлемента = Тип("Массив") Тогда + Для Каждого Э Из Параметр Цикл + Если ТипЗнч(Э) <> Тип("ПараметрБенчмарка") Тогда + ВызватьИсключение СтрШаблон("Ожидался ПараметрБенчмарка внутри массива, получено <%1>", ТипЗнч(Э)); + КонецЕсли; + КонецЦикла; + КонецЕсли;
77-123: Комбинатор групп — ок; сложность приемлемаАлгоритм Картежи×Группы реализован правильно. При необходимости можно микроподправить копирование комбинаций через .Скопировать() (если доступно) для читаемости.
src/BenchmarkOneScript/core/Классы/СериализаторНастроекБенчмарков.os (1)
20-39: Запись настроек в JSON — ок; добавьте try/finally и проверку каталога
- Оберните работу с файлом в try/finally для гарантированного закрытия.
- Убедитесь, что каталог для ИмяФайла существует (создайте при необходимости).
-ЗаписьJSON.ОткрытьФайл(ИмяФайла, "UTF-8", , ПараметрыЗаписиJSON); -ЗаписьJSON.ЗаписатьБезОбработки(...); -ЗаписьJSON.Закрыть(); +// ФС.СоздатьКаталог(ПолучитьКаталог(ИмяФайла)); // если требуется +Попытка + ЗаписьJSON.ОткрытьФайл(ИмяФайла, "UTF-8", , ПараметрыЗаписиJSON); + ЗаписьJSON.ЗаписатьБезОбработки(...); +Исключение + // _Лог.Ошибка(...) по желанию +КонецПопытки; +Если ЗначениеЗаполнено(ЗаписьJSON) Тогда ЗаписьJSON.Закрыть(); КонецЕсли;src/BenchmarkOneScript/core/Модули/Компаратор.os (1)
51-66: Consider checking for key existence.The function doesn't verify that keys from
Соответствие1exist inСоответствие2before comparing values. If a key is missing,Соответствие2[Ключ]will returnНеопределено, which may not be the intended behavior.Consider adding a key existence check:
Для Каждого КлючЗначение Из Соответствие1 Цикл Ключ = КлючЗначение.Ключ; + Если Соответствие2[Ключ] = Неопределено И Не Соответствие1[Ключ] = Неопределено Тогда + Возврат Ложь; + КонецЕсли; Если Не Совпадают(Соответствие1[Ключ], Соответствие2[КлючЗначение.Ключ]) Тогда Возврат Ложь; КонецЕсли; КонецЦикла;src/BenchmarkOneScript/jason/Классы/ДесериализаторJsonBench.os (2)
117-154: Типы столбцов ТаблицыЗначений игнорируютсяАргумент ТипЭлементов не используется; все значения десериализуются по их фактическому типу. Это ломает строгую схему и симметрию с сериализацией.
Минимально: если ТипЭлементов передан как массив типов той же длины, приводить значения по колонкам:
- ПреобразованныйЭлемент = ПреобразоватьОбъектДесериализации(ЗначениеКолонки, ТипЗнч(ЗначениеКолонки)); + ОжидаемыйТипКолонки = ?(ЗначениеЗаполнено(ТипЭлементов) И ТипЗнч(ТипЭлементов)=Тип("Массив") И ИндексКолонки < ТипЭлементов.Количество(), ТипЭлементов[ИндексКолонки], ТипЗнч(ЗначениеКолонки)); + ПреобразованныйЭлемент = ПреобразоватьОбъектДесериализации(ЗначениеКолонки, ОжидаемыйТипКолонки);
183-191: Неизвестные поля пользовательских типов молча игнорируютсяСейчас поля без соответствия в классе просто пропускаются. Это затрудняет поиск ошибок конфигурации/опечаток.
Добавьте настраиваемую строгость (флаг в модуле/параметр функции) и как минимум логируйте на уровне предупреждения:
- // Если поле не найдено, то пропускаем его - // TODO: Падать, если не висит @JsonIgnore - Продолжить; + Лог.Предупреждение("Поле '%1' отсутствует в типе %2 и будет проигнорировано", КлючИЗначение.Ключ, ТипОбъекта); + Продолжить;Опционально — выбрасывать исключение в строгом режиме.
src/BenchmarkOneScript/cmd/Классы/КомандаЗапуска.os (2)
218-233: Парсинг экспортеров: принять 'md' и нормализовать вводОписание CLI обещает 'md', но маппинг ждёт 'markdown'. Также разделение по ", " ломается без пробела.
- Сплит по ',' без пробела.
- Трим пробелы и нормализуйте регистр.
- Добавьте синоним 'md'.
- ДопустимыеЗначения.Вставить("markdown", "Markdown"); + ДопустимыеЗначения.Вставить("markdown", "Markdown"); + ДопустимыеЗначения.Вставить("md", "Markdown"); ... - Для Каждого Экспортер Из СтрРазделить(_Экспортеры, ", ", Ложь) Цикл - - Имя = ДопустимыеЗначения[НРег(Экспортер)]; + Для Каждого СыройЭкспортер Из СтрРазделить(_Экспортеры, ",", Ложь) Цикл + Экспортер = НРег(СтрЗаменить(СыройЭкспортер, " ", "")); + Имя = ДопустимыеЗначения[Экспортер];
104-113: Чтение настроек без обработки ошибокИсключения из ПрочитатьИзJson завершат команду целиком без контекста.
Добавьте обработку для улучшения UX:
Функция ПрочитатьНастройки() Если Не ЗначениеЗаполнено(_ФайлНастроек) Тогда Возврат Неопределено; КонецЕсли; - Сериализатор = Новый СериализаторНастроекБенчмарков(); - Возврат Сериализатор.ПрочитатьИзJson(_ФайлНастроек); + Сериализатор = Новый СериализаторНастроекБенчмарков(); + Попытка + Возврат Сериализатор.ПрочитатьИзJson(_ФайлНастроек); + Исключение + _Лог.Ошибка("Не удалось прочитать файл настроек <%1>: %2", _ФайлНастроек, ОписаниеОшибки()); + Возврат Неопределено; + КонецПопытки; КонецФункцииsrc/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os (1)
6-7: Лишняя зависимостьcollectionos не используется в модуле.
Удалите импорт для уменьшения связности:
-#Использовать collectionossrc/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os (1)
1148-1156: Сериализация обработчиков: типы-делегаты не поддерживаютсяСейчас бросаете исключение, если Обработчик не строка. Это делает сериализацию неснимемой при программной конфигурации.
Оставить как есть, но улучшить сообщение: добавить имя события и подсказку использовать строковые имена. Или конвертировать Делегат в строку-имя при наличии .ИмяМетода().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
docs/CLI.md(1 hunks)packagedef(4 hunks)src/BenchmarkOneScript/cmd/Классы/КомандаЗапуска.os(4 hunks)src/BenchmarkOneScript/core/Классы/ДескрипторБенчмарка.os(8 hunks)src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os(8 hunks)src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарковИзКаталога.os(1 hunks)src/BenchmarkOneScript/core/Классы/КоллекцияДескрипторовБенчмарков.os(3 hunks)src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os(12 hunks)src/BenchmarkOneScript/core/Классы/ПараметрБенчмарка.os(1 hunks)src/BenchmarkOneScript/core/Классы/ПараметрыМетодаБенчмарка.os(1 hunks)src/BenchmarkOneScript/core/Классы/СериализаторКонфигурацииБенчмарков.os(0 hunks)src/BenchmarkOneScript/core/Классы/СериализаторНастроекБенчмарков.os(1 hunks)src/BenchmarkOneScript/core/Классы/СериализаторРезультатовБенчмарков.os(3 hunks)src/BenchmarkOneScript/core/Классы/ЭкспортерРезультатовБенчмарковВJson.os(1 hunks)src/BenchmarkOneScript/core/Модули/ИзвлекательДескрипторовБенчмарков.os(1 hunks)src/BenchmarkOneScript/core/Модули/ИзвлекательКонфигурацииБенчмарков.os(1 hunks)src/BenchmarkOneScript/core/Модули/КомбинаторПараметровБенчмарка.os(4 hunks)src/BenchmarkOneScript/core/Модули/Компаратор.os(1 hunks)src/BenchmarkOneScript/dto/Классы/НастройкиБенчмарковДто.os(1 hunks)src/BenchmarkOneScript/jason/Классы/АннотацияНесериализуемое.os(1 hunks)src/BenchmarkOneScript/jason/Классы/АннотацияСериализуемое.os(1 hunks)src/BenchmarkOneScript/jason/Классы/ДесериализаторJsonBench.os(1 hunks)src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os(1 hunks)tests/fixtures/Классы/БенчмаркСАннотациямиКонфигурации.os(1 hunks)tests/ТестыБенчмарков.os(3 hunks)tests/ТестыКонфигурации.os(2 hunks)tests/ТестыСериализаторБенчмарков.os(1 hunks)tests/ТестыЭталонов.os(1 hunks)
💤 Files with no reviewable changes (1)
- src/BenchmarkOneScript/core/Классы/СериализаторКонфигурацииБенчмарков.os
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: sonar / test
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: sonar / test
🔇 Additional comments (39)
src/BenchmarkOneScript/jason/Классы/АннотацияНесериализуемое.os (1)
1-9: Annotation looks correct.Declaration is minimal and consistent with annotation pattern.
Please add/confirm a test where a field marked
&Несериализуемоеis excluded even if&Сериализуемоеis present elsewhere in the type.src/BenchmarkOneScript/core/Модули/ИзвлекательДескрипторовБенчмарков.os (2)
3-3: Good: explicit reflector import.Keeps dependency clear and avoids implicit availability issues.
10-10: Param doc clarification LGTM.Accepts both type and instance; matches
РефлекторОбъектаusage.src/BenchmarkOneScript/core/Модули/ИзвлекательКонфигурацииБенчмарков.os (1)
2-2: Reflector import LGTM.Aligns with new reflection-based extraction.
docs/CLI.md (1)
26-26: Review comment is incorrect and should be disregarded.The review references deprecating
-cand--configCLI flags, but these flags do not exist in the codebase. The complete CLI flag list indocs/CLI.md(lines 15-26) shows only:--iterationCount,--iterationTime,--warmupCount,--invocationCount,--runtime,--throughput,--coldstart,-m/--memory,-e/--exporters,-a/--artifacts,-r/--recursive, and--settings. The codebase already uses--settingsexclusively (e.g., insrc/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.oslines 196-197). No references to-cor--configas CLI flags were found anywhere in the code.Likely an incorrect or invalid review comment.
tests/fixtures/Классы/БенчмаркСАннотациямиКонфигурации.os (1)
43-46: Параметры и источник: проверьте наличие источника и согласованность контракта.
- В файле нет реализации функций-источников «ПолучитьПараметры»/«ПолучитьЗначения». Если фикстура реально вызывается извлекателем, добавьте заглушку или переименуйте атрибут под существующую функцию.
- Уточните приоритет: как объединяются &Параметры на уровне класса (стр. 1) и метода (стр. 43–45)? Избегите дублирования/конфликтов.
- Метод «Бенчмарк(Парам)» — параметр не используется. Убедитесь, что извлекатель/раннер не требует соответствия числу аннотированных параметров.
Можно подтвердить контрактом/тестом, что извлекатель корректно работает при отсутствии источника в этой фикстуре?
src/BenchmarkOneScript/core/Классы/КоллекцияДескрипторовБенчмарков.os (2)
3-6: Аннотации сериализации на поле коллекции — LGTM.Хорошо, что задано имя «Элементы», тип и итерация для сериализации.
55-59: Очистить() — уместно.Простая и ожидаемая операция для переиспользования коллекции.
packagedef (2)
10-10: Адрес репозитория — отлично.Добавление .АдресРепозитория улучшает метаданные пакета.
21-38: Verify compatibility of dependency updates with OneScript 1.9.3.The packages are actively used in the codebase:
reflector(0.7.1): ДескрипторБенчмарка.os, КонфигурацияБенчмарков.os, ИзвлекательКонфигурацииБенчмарков.os, ИзвлекательДескрипторовБенчмарков.osvalidate(0.3.0): ДесериализаторJsonBench.os, ДескрипторБенчмарка.os, КонфигурацияБенчмарков.oscollectionos(0.8.2): СериализаторJsonBench.osConfirm:
- Minimum OneScript engine version for reflector 0.7.1, collectionos 0.8.2, validate 0.3.0 supports OneScript 1.9.3
- Packages are available in OneScript OPM registry
- No known security vulnerabilities in these versions
tests/ТестыБенчмарков.os (2)
156-156: Переход на объектную модель параметров подтверждёнИспользование НаборыПараметров[0].Получить(0).ЭтоПараметрМетода выглядит корректно и соответствует новой модели.
775-779: Актуализированные проверки значений параметровПереход на .Получить(i).Значение согласован с новой структурой набора параметров.
tests/ТестыКонфигурации.os (1)
14-16: Переименование helper‑процедуры — окВызов ПроверитьКонфигурациюБенчмаркаСАннотациямиКонфигурации(…) соответствует новой экспортируемой процедуре.
src/BenchmarkOneScript/core/Модули/КомбинаторПараметровБенчмарка.os (2)
7-9: Поддержка ПараметрыМетодаБенчмарка — окДокстринг и контракт обновлены корректно.
131-138: Обработка ПараметрыМетодаБенчмарка встраивается корректноДелегирование на ДобавитьПараметрыВКомбинацию с Параметр.ВМассив() — верно.
src/BenchmarkOneScript/core/Классы/ПараметрыМетодаБенчмарка.os (4)
1-11: LGTM!The serialization annotations and constructor are correctly implemented. The use of
&Сериализуемоеwith&Тип("Массив")and&ДляКаждогоproperly defines the collection structure for serialization.
13-24: LGTM!The validation logic correctly ensures that only
ПараметрБенчмаркаinstances withЭтоПараметрМетода = Истинаare added to the collection. This maintains the integrity of the parameter collection.
26-43: LGTM!Standard accessor methods with proper documentation. The implementation correctly delegates to the internal array.
45-51: LGTM!The method creates a defensive copy by wrapping the internal array in a
ФиксированныйМассивand then a newМассив. This prevents external modifications from affecting the collection's internal state while providing a usable array to callers.tests/ТестыСериализаторБенчмарков.os (3)
1-18: LGTM!Standard test setup with proper resource management. The temporary file manager is correctly initialized in setup and cleaned up in teardown.
20-36: LGTM!The test correctly validates the serialization roundtrip by writing to JSON and reading back, then verifying the structure. This is the right approach for testing serialization fidelity.
61-104: LGTM!Comprehensive validation of the deserialized configuration. The test covers exporters, runtime versions, columns, parameters, event handlers, and artifact paths. Well-structured test helper.
src/BenchmarkOneScript/core/Модули/Компаратор.os (3)
35-49: LGTM!The array comparison correctly short-circuits on count mismatch and recursively compares elements. The use of
Совпадаютfor recursive comparison supports nested structures.
68-87: LGTM!The structure comparison correctly checks for property existence in the second structure using
Свойство()before comparing values. This is the right approach for structure comparison.
89-114: LGTM!The table comparison logic is well-structured. It validates column structure first, then performs row-by-row comparison. The column existence check at lines 96-100 prevents runtime errors.
src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os (6)
196-198: LGTM!The CLI flag change from
--configto--settingsaligns with the PR objectives and the updated documentation in docs/CLI.md.
241-250: LGTM!Correct usage of the new
СериализаторНастроекБенчмарковclass and itsЗаписатьВJsonmethod. The serialization includes both descriptors and configuration as required.
280-295: LGTM!The updated deserialization call passes
_КонфигурацияtoПрочитатьИзJson, which provides necessary context for deserializing benchmark results.
685-717: LGTM!The conversion logic correctly wraps parameters in the new
ПараметрыМетодаБенчмаркаcollection type. The type dispatch handles arrays, structures, maps, and table rows appropriately.
719-741: LGTM!The array-to-parameters conversion properly validates parameter count and creates individual
ПараметрБенчмаркаinstances for each array element, wrapping them in the collection.
743-796: LGTM!Both conversion helpers (
ПреобразоватьСтруктуруВПараметрыМетодаБенчмаркаandПреобразоватьСтрокуТаблицыЗначенийВПараметрыМетодаБенчмарка) correctly validate parameter presence and create properly structured parameter collections. The consistent pattern across all conversion helpers makes the code maintainable.src/BenchmarkOneScript/core/Классы/ДескрипторБенчмарка.os (6)
1-35: LGTM!The serialization annotations are well-structured. Marking
_ИменаПараметровas&Несериализуемоеis correct since this derived data is re-extracted duringПослеДесериализации().
96-143: LGTM!The refactoring to use
ПараметрыМетодаБенчмаркаcorrectly groups parameters into sets. Both array and single-value cases are handled appropriately, maintaining backward compatibility in behavior while changing the internal structure.
272-278: LGTM!The post-deserialization hook correctly validates the deserialized data and reconstructs derived state (
_ИменаПараметров). Creating a new instance viaНовый(_Тип)for reflection is the appropriate approach.
285-294: LGTM!The parameter name extraction uses reflection appropriately to get method metadata and extract parameter names. This is a clean implementation.
296-305: LGTM!The validation procedure correctly uses the
validatelibrary to validate the descriptor state and provides clear error messages viaОписаниеОшибокВалидации.
44-57: No breaking change - constructor signature change is backward compatible.Both existing calls to the constructor in the codebase pass exactly two arguments that map to the parameters. Making
ОбъектandМетодoptional with defaults does not break these calls—they continue working as before. The change safely enables the new deserialization pattern without affecting existing code.src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os (1)
137-141: Неизвестные пользовательские типы: только предупреждение, результат НеопределеноЭто может дать "null" в JSON без явной ошибки на стороне вызывающего кода.
Подтвердите желаемую политику. Варианты:
- Строгий режим: бросать исключение.
- Мягкий: оставлять предупреждение, но также включать имя типа в выход (напр., объект-обёртку с метаданными).
Если нужен строгий режим — могу предложить патч с флагом.src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os (1)
1068-1079: Определение модифицированных полей: корректно и лаконичноХорошее использование Компаратор.Совпадают и кэшированных описаний.
| Процедура ПередСборкой(РабочийКаталог) Экспорт | ||
| ПодготовкаКомпонентов = ЗагрузитьСценарий("build/ПодготовкаКомпонентов.os"); | ||
| ПодготовкаКомпонентов.Подготовить(); | ||
| КонецПроцедуры |
There was a problem hiding this comment.
Удалите BOM в начале файла.
Символ BOM () в строке 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.
| Процедура ПередСборкой(РабочийКаталог) Экспорт | |
| ПодготовкаКомпонентов = ЗагрузитьСценарий("build/ПодготовкаКомпонентов.os"); | |
| ПодготовкаКомпонентов.Подготовить(); | |
| КонецПроцедуры | |
| Процедура ПередСборкой(РабочийКаталог) Экспорт | |
| ПодготовкаКомпонентов = ЗагрузитьСценарий("build/ПодготовкаКомпонентов.os"); | |
| ПодготовкаКомпонентов.Подготовить(); | |
| КонецПроцедуры |
🤖 Prompt for AI Agents
In packagedef around lines 1 to 4, the file begins with a BOM character ()
which can break some tools; remove the BOM so the file starts with the first
Cyrillic character, re-save the file as UTF-8 without BOM (or strip the U+FEFF
at the very start), and verify there are no other hidden/startup bytes before
"Процедура".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/BenchmarkOneScript/core/Модули/Компаратор.os (1)
51-66: Missing explicit key existence check can cause incorrect equality.Unlike
СтруктурыСовпадают(line 76) which properly checksСтруктура2.Свойство(Ключ), this function accessesСоответствие2[Ключ]without verifying the key exists. In OneScript, accessing a non-existent key returnsНеопределено, which can lead to false positives when comparing maps withНеопределеноvalues.Example:
- Соответствие1 =
{A: Неопределено}(count = 1)- Соответствие2 =
{B: Неопределено}(count = 1)- When checking key A: both return
Неопределено, incorrectly matching despite different keys.Consider adding an explicit iteration over keys in both directions or collecting keys for comparison:
Функция СоответствияСовпадают(Соответствие1, Соответствие2) Если Соответствие1.Количество() <> Соответствие2.Количество() Тогда Возврат Ложь; КонецЕсли; Для Каждого КлючЗначение Из Соответствие1 Цикл Ключ = КлючЗначение.Ключ; + // Проверяем существование ключа во втором соответствии + Если Не СоответствиеСодержитКлюч(Соответствие2, Ключ) Тогда + Возврат Ложь; + КонецЕсли; + Если Не Совпадают(Соответствие1[Ключ], Соответствие2[Ключ]) Тогда Возврат Ложь; КонецЕсли; КонецЦикла; Возврат Истина; КонецФункции + +// Helper function to check if key exists (since values can be Неопределено) +Функция СоответствиеСодержитКлюч(Соответствие, Ключ) + Для Каждого КлючЗначение Из Соответствие Цикл + Если КлючЗначение.Ключ = Ключ Тогда + Возврат Истина; + КонецЕсли; + КонецЦикла; + Возврат Ложь; +КонецФункции
🧹 Nitpick comments (4)
src/BenchmarkOneScript/cmd/Классы/КомандаЗапуска.os (3)
86-90: Simplify condition for consistency.The double negative
Не Настройки = Неопределеноis less readable thanНастройки <> Неопределено(which you use at line 172).Apply this diff:
- Если Не Настройки = Неопределено И Настройки.ДескрипторыБенчмарков.Количество() > 0 Тогда + Если Настройки <> Неопределено И Настройки.ДескрипторыБенчмарков.Количество() > 0 Тогда
92-100: Consider logging a summary of execution results.Errors are logged per-type, but there's no summary at the end showing which types succeeded or failed. For multiple benchmark types, this could make it harder to identify problems at a glance.
Consider tracking execution results and logging a summary after the loop, e.g.:
УспешныеТипы = Новый Массив(); ПроваленныеТипы = Новый Массив(); Для Каждого Тип Из ПодключенныеТипы Цикл Попытка Бенчмаркинг.Запустить(Источник, КонфигурацияТипа); УспешныеТипы.Добавить(Тип); Исключение _Лог.Ошибка("..."); ПроваленныеТипы.Добавить(Тип); КонецПопытки; КонецЦикла; Если ПроваленныеТипы.Количество() > 0 Тогда _Лог.Предупреждение("Завершено с ошибками. Успешно: %1, Провалено: %2", УспешныеТипы.Количество(), ПроваленныеТипы.Количество()); КонецЕсли;
133-146: Past issue resolved: Self-modification during iteration fixed.The previous review comment about self-modification has been properly addressed. The code now uses a separate
НайденныеФайлыvariable (line 135) to avoid modifyingФайлыduring iteration.However, note that duplicate checking using
Массив.Найти()at lines 137 and 143 results in O(n²) complexity. For typical benchmark use cases, this is acceptable, but if you expect hundreds of files, consider using aСоответствиеfor O(1) lookup.src/BenchmarkOneScript/core/Модули/Компаратор.os (1)
68-87: LGTM! Structure comparison correctly checks key existence.The explicit
Структура2.Свойство(Ключ)check (line 76) properly verifies the key exists before comparing values, which is the correct approach.Minor: Line 80 could use the local
Ключvariable instead ofКлючЗначение.Ключfor consistency:- Если Не Совпадают(Структура1[Ключ], Структура2[КлючЗначение.Ключ]) Тогда + Если Не Совпадают(Структура1[Ключ], Структура2[Ключ]) Тогда
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/BenchmarkOneScript/cmd/Классы/КомандаЗапуска.os(3 hunks)src/BenchmarkOneScript/core/Классы/СериализаторНастроекБенчмарков.os(1 hunks)src/BenchmarkOneScript/core/Модули/ИзвлекательКонфигурацииБенчмарков.os(2 hunks)src/BenchmarkOneScript/core/Модули/Компаратор.os(1 hunks)src/BenchmarkOneScript/dto/Классы/НастройкиБенчмарковДто.os(1 hunks)tests/ТестыСериализаторБенчмарков.os(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/BenchmarkOneScript/core/Классы/СериализаторНастроекБенчмарков.os
- tests/ТестыСериализаторБенчмарков.os
- src/BenchmarkOneScript/dto/Классы/НастройкиБенчмарковДто.os
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: sonar / test
- GitHub Check: sonar / test
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (stable) / build (windows-latest, stable)
🔇 Additional comments (10)
src/BenchmarkOneScript/core/Модули/ИзвлекательКонфигурацииБенчмарков.os (4)
2-2: LGTM! Missing import added.The reflector package is used at line 22 (
РефлекторОбъекта), so this import declaration is necessary and fixes a missing dependency.
9-9: LGTM! Documentation clarity improved.Removing the redundant "Тип, " prefix makes the parameter documentation clearer and more concise.
37-43: LGTM! Consistency improved and potential null reference prevented.Using
КонфигурацияБенчмарковinstead ofКонфигурацияensures a valid object is always passed. WhenКонфигурация = Неопределено(line 16), passingКонфигурацияwould result in passingНеопределеноto these functions, causing errors when they invoke methods on the configuration. This change aligns with lines 29-36 which already useКонфигурацияБенчмарков.
51-54: LGTM! Consistency completed for event handlers.Same fix as lines 37-43: using
КонфигурацияБенчмарковensures a valid configuration object is passed toПрочитатьОбработчикиСобытия, preventing potential null reference errors. This completes the consistency improvement across the entireИзвлечьfunction.src/BenchmarkOneScript/cmd/Классы/КомандаЗапуска.os (3)
47-49: LGTM: Clean migration to settings-based configuration.The rename from
--configto--settingsaligns with the PR objective to introduce JSON-based settings.
65-69: LGTM: Logger initialization is correct.Logger properly declared and initialized for error reporting.
170-176: LGTM: Configuration initialization handles both settings and defaults.The function correctly initializes configuration from
Настройкиwhen provided, or creates a default configuration otherwise. CLI options are then applied to override settings.src/BenchmarkOneScript/core/Модули/Компаратор.os (3)
11-29: LGTM! Well-structured dispatch logic.The type-based dispatch pattern is clean and handles the main collection types appropriately. The early type-mismatch check (line 15) is efficient, and the fallback to simple equality for primitives is correct.
35-49: LGTM! Array comparison is correct.The length check followed by element-wise recursive comparison is the standard approach for deep array equality.
89-114: LGTM! Table comparison is comprehensive.The implementation properly verifies:
- Row and column count equality (lines 91-92)
- Column name matching (lines 96-100)
- Cell-by-cell recursive comparison (lines 102-110)
This ensures both schema and data equality for value tables.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BenchmarkOneScript/core/Классы/ДескрипторБенчмарка.os (1)
44-57: Critical issue confirmed: constructor fails when passed a Type object.The verification shows the bug is real. In
СериализаторРезультатовБенчмарков.osline 330,Типis created from a string:Тип = Тип(ДанныеБенчмарк.Тип);, making it a Type object. This Type object is then passed to the constructor at line 342.The constructor at line 46 correctly accepts both Type and instance objects, but line 54 unconditionally calls
ИзвлечьИменаПараметров(Объект)wheneverОбъектis not Undefined. The procedureИзвлечьИменаПараметров(line 287) createsРефлекторОбъекта(Объект), which requires an instance, not a Type.Fix required: Add type checking before calling
ИзвлечьИменаПараметровto skip this call whenОбъектis a Type:Если Не Объект = Неопределено И ТипЗнч(Объект) <> Тип("Тип") Тогда ИзвлечьИменаПараметров(Объект); КонецЕсли;
♻️ Duplicate comments (1)
src/BenchmarkOneScript/jason/Классы/ДесериализаторJsonBench.os (1)
99-101: Преобразование Тип(Строка) без защиты.Ранее уже поднималось. Нужна обертка с внятным сообщением.
Функция ПреобразоватьТип(Объект) - Возврат Тип(Объект); + Попытка + Возврат Тип(Объект); + Исключение + ВызватьИсключение СтрШаблон("Неверное значение типа: %1", Объект); + КонецПопытки; КонецФункции
🧹 Nitpick comments (7)
tests/ТестыКонфигурации.os (2)
748-793: Укрепить проверки: не полагаться на порядок параметров и покрыть новые поля.Сейчас ассерты опираются на индексы Параметры[0]/[1]. Это хрупко при изменении порядка. Рекомендую:
- Искать параметры по Имя или Значение через ПроцессорыКоллекций.
- Дополнительно проверить новые свойства ПараметраБенчмарка (Имя не пусто, ЭтоПараметрМетода = Ложь).
Пример вставки (после проверки длины 2):
+ // Параметры: не полагаться на порядок и проверить новые поля + Парам100 = ПроцессорыКоллекций.ИзКоллекции(Параметры).Первый("П -> П.Значение = 100"); + Ожидаем.Что(Парам100, "Параметр со значением 100").Не_().ЭтоНеопределено(); + Ожидаем.Что(Парам100.Имя, "Имя параметра").Заполнено(); + Ожидаем.Что(Парам100.ЭтоПараметрМетода, "Флаг параметра метода").Равно(Ложь);Это снимет зависимость от порядка и заодно валидирует новую модель.
786-786: Расширить типовую проверку параметра.Помимо ИмеетТип("ПараметрБенчмарка"), имеет смысл проверить заполненность Имя и дефолт Ложь для ЭтоПараметрМетода.
src/BenchmarkOneScript/core/Классы/КоллекцияДескрипторовБенчмарков.os (1)
18-20: Поддержать fluent-интерфейс у Очистить/Прочитать.Файл уже использует fluent; логично возвращать ЭтотОбъект для цепочек.
-Процедура Очистить() Экспорт - _ДескрипторыБенчмарков.Очистить(); -КонецПроцедуры +Функция Очистить() Экспорт + _ДескрипторыБенчмарков.Очистить(); + Возврат ЭтотОбъект; +КонецФункции -Процедура Прочитать(ИсточникБенчмарков) Экспорт - Очистить(); - ИзвлекательДескрипторовБенчмарков.Извлечь(ИсточникБенчмарков, ЭтотОбъект); -КонецПроцедуры +Функция Прочитать(ИсточникБенчмарков) Экспорт + Очистить(); + ИзвлекательДескрипторовБенчмарков.Извлечь(ИсточникБенчмарков, ЭтотОбъект); + Возврат ЭтотОбъект; +КонецФункцииAlso applies to: 55-59, 131-139
src/BenchmarkOneScript/jason/Классы/ДесериализаторJsonBench.os (2)
175-219: Кэшировать метаданные свойств пользовательских типов.Рефлексия для каждого объекта (ПолучитьТаблицуСвойств и построение карты имен) дорогая. Добавьте кэш по ТипОбъекта.
Эскиз:
-Перем КэшИзвестныхТипов; +Перем КэшИзвестныхТипов; +Перем КэшСвойствТипа; // Соответствие: Тип -> {Свойства, КартаИменПолей} Процедура ПриСозданииОбъекта() ... - КэшИзвестныхТипов = Новый МножествоСоответствие; + КэшИзвестныхТипов = Новый МножествоСоответствие; + КэшСвойствТипа = Новый Соответствие(); Функция ПреобразоватьПользовательскийТип(Объект, ТипОбъекта) - Результат = Новый(ТипОбъекта); - СвойстваОбъекта = Рефлектор.ПолучитьТаблицуСвойств(ТипОбъекта, Истина); - СвойстваПоИменамПолей = Новый Соответствие(); + Результат = Новый(ТипОбъекта); + Кэш = КэшСвойствТипа[ТипОбъекта]; + Если Кэш = Неопределено Тогда + СвойстваОбъекта = Рефлектор.ПолучитьТаблицуСвойств(ТипОбъекта, Истина); + СвойстваПоИменамПолей = Новый Соответствие(); + Для Каждого Свойство Из СвойстваОбъекта Цикл + АннотацияСериализуемое = РаботаСАннотациями.ПолучитьАннотацию(Свойство, "Сериализуемое"); + ИмяСвойства = РаботаСАннотациями.ПолучитьЗначениеПараметраАннотации(АннотацияСериализуемое, "Значение", Свойство.Имя); + СвойстваПоИменамПолей.Вставить(ИмяСвойства, Свойство); + КонецЦикла; + Кэш = Новый Структура("Свойства,Карта", СвойстваОбъекта, СвойстваПоИменамПолей); + КэшСвойствТипа.Вставить(ТипОбъекта, Кэш); + КонецЕсли; + СвойстваОбъекта = Кэш.Свойства; + СвойстваПоИменамПолей = Кэш.Карта;
221-263: Не пересоздавать контейнер аннотаций каждый раз.КонтейнерАннотаций статичен. Инициализируйте его один раз в ПриСозданииОбъекта и переиспользуйте.
src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os (2)
111-149: Constructor logic is functional but somewhat indirect.The constructor establishes defaults by:
- Calling setters (lines 133-140), which mark fields as modified
- Clearing modification flags (line 142)
- Remembering values as defaults (line 143)
This works and ensures defaults pass through setter validation. However, it's indirect: setters are called to set defaults, then their side effect (marking modified) is cleared. A more direct approach would be to initialize fields without triggering modification tracking, but the current pattern is acceptable if it ensures consistency.
1098-1129: Field metadata extraction depends on naming convention.Line 1112 uses
Сред(Свойство.Имя, 2)to derive the public field name by stripping the leading"_". This assumes all internal fields follow the exact pattern_ИмяПоля.If any field is named differently (e.g.,
__Field,Field, or_m_Field), the extraction will produce incorrect names. Document this naming requirement or add validation to detect violations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/BenchmarkOneScript/core/Классы/ДескрипторБенчмарка.os(8 hunks)src/BenchmarkOneScript/core/Классы/КоллекцияДескрипторовБенчмарков.os(3 hunks)src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os(12 hunks)src/BenchmarkOneScript/core/Классы/ПараметрБенчмарка.os(1 hunks)src/BenchmarkOneScript/core/Классы/ПараметрыМетодаБенчмарка.os(1 hunks)src/BenchmarkOneScript/dto/Классы/НастройкиБенчмарковДто.os(1 hunks)src/BenchmarkOneScript/jason/Классы/АннотацияСериализуемыйКлюч.os(1 hunks)src/BenchmarkOneScript/jason/Классы/ДесериализаторJsonBench.os(1 hunks)src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os(1 hunks)tests/ТестыКонфигурации.os(3 hunks)tests/ТестыСериализаторБенчмарков.os(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/BenchmarkOneScript/core/Классы/ПараметрыМетодаБенчмарка.os
- tests/ТестыСериализаторБенчмарков.os
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: sonar / test
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: sonar / test
🔇 Additional comments (13)
src/BenchmarkOneScript/jason/Классы/АннотацияСериализуемыйКлюч.os (1)
9-10: No changes required—empty procedure bodies are intentional and consistent across all annotation classes.The codebase uses a marker-based annotation pattern where all similar annotation classes (
АннотацияСериализуемое,АннотацияНесериализуемое) also have emptyПриСозданииОбъектаprocedure bodies, even when parameters are declared. This confirms the annotations work through reflection and metadata presence rather than runtime initialization logic.tests/ТестыКонфигурации.os (1)
14-14: Переименование вызова — ок.Вызов обновлен на ПроверитьКонфигурациюБенчмаркаСАннотациямиКонфигурации — без замечаний.
src/BenchmarkOneScript/core/Классы/КоллекцияДескрипторовБенчмарков.os (1)
3-7: Аннотации сериализации — ок.Схема &Сериализуемое("Items") + &Тип("Массив") + &ДляКаждого &Тип("ДескрипторБенчмарка") выглядит корректно для JSON-массива элементов.
src/BenchmarkOneScript/core/Классы/ПараметрБенчмарка.os (1)
3-11: LGTM: сериализация и значения по умолчанию.Ключи JSON (Name/Value/IsMethodParameter) и дефолты (""/Неопределено/Ложь) заданы корректно и совместимы с отсутствующими полями при десериализации.
Also applies to: 12-18
src/BenchmarkOneScript/dto/Классы/НастройкиБенчмарковДто.os (1)
1-14: LGTM: DTO-контейнер.Поле-дескрипторы и конфигурация инициализируются и помечены для сериализации. Без замечаний.
src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os (2)
138-184: LGTM with note on exception handling.The complex type serialization logic correctly:
- Validates known types and logs warnings for unknown types
- Invokes
ПередСериализациейlifecycle hook when present- Processes
Несериализуемое,Сериализуемое, andОбязательноеannotations- Recursively serializes property values
Note that if
ПередСериализацией(line 154) throws an exception, it will propagate uncaught. If fail-fast behavior is intended, this is correct; otherwise, consider whether lifecycle errors should be wrapped with context.
206-225: LGTM.Constructor correctly initializes reflector, logger, and type caches. Note that
КэшИзвестныхТиповis populated with all known user types at construction time, so dynamically loaded types afterward won't be recognized. This is likely acceptable for the expected usage pattern.src/BenchmarkOneScript/core/Классы/ДескрипторБенчмарка.os (2)
270-279: LGTM.Post-deserialization correctly:
- Validates the deserialized descriptor
- Reconstructs
_ИменаПараметров(markedНесериализуемое) by creating a new instance from_Типand extracting parameter namesThe approach is sound for restoring non-serialized derived state.
96-143: LGTM.Parameter handling correctly refactored to use
ПараметрыМетодаБенчмаркаas a container forПараметрБенчмаркаinstances. The logic maintains backward compatibility for both array and single-value parameter inputs.src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os (4)
1155-1205: LGTM.Serialization lifecycle correctly:
- Validates that event handlers and parameter sources are serializable (string-typed, not delegates) before serialization
- Converts exporters to type strings for serialization and reconstructs them after deserialization
- Validates configuration integrity after deserialization
The pattern ensures a successful serialization round-trip.
563-620: LGTM.Event handler methods correctly updated to use renamed field
_Обработчики(previously_ОбработчикиСобытий). The public API maintains backward compatibility through theОбработчикиСобытий()getter, while the serialized name is "Обработчики" per line 1360.
975-984: LGTM.Refactored to use metadata-driven approach via
ОписанияПолей()instead of manually constructing rows. This reduces duplication and ensures the field list is maintained in one place through annotations.
1075-1096: Компаратор.Совпадают implementation is comprehensive and correctly handles all configuration field types.The Компаратор module provides dedicated handlers for collection types and recursive comparison for nested structures:
- ТаблицаЗначений receives row-by-row, column-by-column comparison
- Arrays use recursive element comparison
- Соответствие and Структура use recursive key/property value comparison
Field inclusion analysis confirms all tracked fields are properly handled:
- _Параметры (Массив из ПараметрБенчмарка): ✓ covered by array handler
- _Обработчики (ТаблицаЗначений): ✓ covered by dedicated table handler
- _Экспортеры (Массив): ✓ covered by array handler (arbitrary types compared via fallback = operator)
src/BenchmarkOneScript/custom-jason/Классы/АннотацияСериализуемыйКлюч.os
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: sonar / test
- GitHub Check: sonar / test
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (dev) / build (macos-latest, dev)
🔇 Additional comments (4)
src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os (4)
30-46: LGTM!The main serialization entry point correctly transforms the object and uses the built-in
ЗаписьJSONfor JSON output. The implementation is clean and straightforward.
52-98: LGTM!The dispatcher correctly routes objects to type-specific converters, and the recursive transformation logic for arrays and structures is implemented properly.
166-180: Verify that required fields with Неопределено values should be serialized.Line 178 includes fields marked as
Обязательноеeven when their value isНеопределено. This means required fields will always appear in the JSON output asnulleven if not set. Confirm this behavior aligns with the serialization requirements.
224-224: No issues found — Тип("Null") is valid OScript syntax.In OScript (1C:Enterprise), there are two distinct special "no value" types: NULL (Null) and Неопределено (Undefined), both documented as valid system types. The code at line 224 correctly uses
Тип("Null")as part of the primitive types set for JSON serialization. This is intentional and appropriate—distinguishing between JSON's null value and OScript's undefined state.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os (1)
745-771: Соответствие: неверная проверка наличия ключа (путает “нет ключа” и “значение = Неопределено”).Сейчас для Соответствие используется сравнение с Неопределено, что срабатывает и при существующем ключе со значением Неопределено. Используйте СодержитКлюч.
Функция ПреобразоватьСтруктуруВПараметрыМетодаБенчмарка(ДескрипторБенчмарка, Параметры) - ПараметрыМетодаБенчмарка = Новый ПараметрыМетодаБенчмарка(); + ПараметрыМетодаБенчмарка = Новый ПараметрыМетодаБенчмарка(); ТипКоллекции = ТипЗнч(Параметры); @@ - Если ТипКоллекции = Тип("Структура") Тогда - ОтсутствуетПараметр = Не Параметры.Свойство(ИмяПараметра); - Иначе - ОтсутствуетПараметр = Параметры[ИмяПараметра] = Неопределено; - КонецЕсли; + Если ТипКоллекции = Тип("Структура") Тогда + ОтсутствуетПараметр = Не Параметры.Свойство(ИмяПараметра); + Иначе // Соответствие + ОтсутствуетПараметр = Не Параметры.СодержитКлюч(ИмяПараметра); + КонецЕсли; @@ - ПараметрБенчмарка = Новый ПараметрБенчмарка(ИмяПараметра, Параметры[ИмяПараметра], Истина); + ПараметрБенчмарка = Новый ПараметрБенчмарка(ИмяПараметра, Параметры[ИмяПараметра], Истина); ПараметрыМетодаБенчмарка.Добавить(ПараметрБенчмарка);
🧹 Nitpick comments (5)
src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os (3)
89-89: Consider limiting exported transformation functions.Several internal transformation functions are marked
Экспорт:
ПреобразоватьСтруктуру(line 89)ПреобразоватьТаблицуЗначений(line 100)ПреобразоватьПримитивныйТип(line 137)ПреобразоватьТип(line 141)ПреобразоватьКомплексныйТип(line 145)If these are exported for testing or advanced use cases, that's fine. However, if they're purely internal helpers, removing
Экспортwould reduce API surface area and give you more flexibility to refactor internals.Also applies to: 100-100, 137-143, 145-145
193-209: Consider validating annotation parameter structure.Lines 201 and 203 access
Аннотация.Параметры[0].ЗначениеandАннотация.Параметры[1].Значениеafter checking the parameter count. If the annotation framework guarantees that parameters always have aЗначениеproperty, this is fine. Otherwise, accessing.Значениеon a malformed parameter object will throw a runtime error.If annotations can be user-defined or loaded from external sources, consider defensive checks:
Если КоличествоПараметров >= 1 И Аннотация.Параметры[0].Свойство("Значение") Тогда // safe to use Аннотация.Параметры[0].Значение КонецЕсли;
38-46: Consider error handling strategy for production use.The serializer currently uses a fail-fast approach with no try-catch blocks. Any error (reflection failure, property access error, JSON writing error) will propagate as an exception, causing serialization to abort entirely.
This is acceptable for benchmark configuration (fail-fast is good for developer tools), but if this serializer is used for production data pipelines or user-facing features, consider:
- Catching and logging specific errors while continuing serialization where possible
- Providing partial results with error indicators
- Offering a strict vs. lenient mode
Also applies to: 52-72, 145-191
src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os (2)
241-250: Новый ЗаписатьНастройкиВФайл — лаконично и по месту.Хорошая инкапсуляция. Подумайте об атомарности записи (временный файл + rename) внутри сериализатора, если файл читается внешним процессом конкурентно.
721-739: Массив -> ПараметрыМетодаБенчмарка: хорошо, но синхронизируйте дефолтный пустой набор.Имплементация корректна. Рекомендация: для консистентности типов заменить в ЗапуститьВсеБенчмарки (Линия 336) «Новый Массив()» на «Новый ПараметрыМетодаБенчмарка()», чтобы fallback не смешивал типы.
- КомбинацииПараметров.Добавить(Новый Массив()); + КомбинацииПараметров.Добавить(Новый ПараметрыМетодаБенчмарка());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os(9 hunks)src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: sonar / test
- GitHub Check: test (dev) / build (macos-latest, dev)
- GitHub Check: test (dev) / build (windows-latest, dev)
- GitHub Check: test (dev) / build (ubuntu-latest, dev)
- GitHub Check: test (stable) / build (ubuntu-latest, stable)
- GitHub Check: test (stable) / build (macos-latest, stable)
- GitHub Check: test (stable) / build (windows-latest, stable)
- GitHub Check: sonar / test
🔇 Additional comments (7)
src/BenchmarkOneScript/jason/Классы/СериализаторJsonBench.os (2)
100-135: Past review issues have been addressed.The three issues flagged in previous reviews are now resolved:
- Typo fixed:
СериализоватьВсеКолонкиis correctly spelled (line 108)- Null guard added:
СериализуемыеКлючиdefaults toНеопределеноand is checked at lines 104-106Вставитьcall fixed: line 201 now correctly passes both key and value parametersAlso applies to: 193-209
145-154: Review comment is accurate; flag for developer verification.The code verification confirms:
- The cache (
КэшИзвестныхТипов) is populated once during initialization (lines 218-222) viaРефлектор.ИзвестныеТипы(), making it static, not dynamic.- Unknown types do return
Неопределеноwith a warning (lines 151-154), as stated.- Your concern about types loaded after serializer initialization not being recognized is valid and important—they will not appear in the cache.
- Mandatory properties are always included (line 183), even if
Неопределено, which is by design.The behavior is intentional but warrants conscious acknowledgment. The developer should confirm this static-cache behavior aligns with their use case.
src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os (5)
776-795: СтрокаТаблицыЗначений -> ПараметрыМетодаБенчмарка: корректно.Проверка наличия колонок и формирование параметров выглядят верно.
175-175: Логика с временным файлом настроек — верифицирована и корректна.Сериализатор
СериализаторНастроекБенчмарковсуществует и правильно реализован. МетодыЗаписатьВJson(Дескрипторы, Конфигурация, ИмяФайла)иПрочитатьИзJson(ИмяФайла)корректно сериализуют/десериализуют настройки. Использование на строке 246 показывает, что временный файл правильно передаётся в дочерний процесс, а на строке 111 в КомандаЗапуска.os настройки успешно читаются обратно. Тесты подтверждают совместимость DTO формата.
284-287: No issues found - return structure is correct.Verification confirms that
ПрочитатьИзJsonreturns a Структура containing the expected "Расшифровка" field. The method builds the return structure at lines 320-322, inserting "Расшифровка" as the table of decoding results. The code at ЗапускательБенчмарков.os:287 correctly accesses this field, and the data structure has not changed.
196-199: No backward compatibility concerns;--settingsflag is properly implemented.Verification confirms:
- The
--settingsflag is defined in the CLI parser (КомандаЗапуска.os line 47) and documented in docs/CLI.md- Exhaustive search of all option decorators in КомандаЗапуска.os shows no old
-cor--configflags exist in this codebase- No deprecation layer, compatibility logic, or migration documentation is present
The code change does not require modification. The
--settingsflag is the implemented design with no legacy alternatives to handle.
695-699: Correctly returns collection type with full framework support.Verification confirms all three concerns in the review are properly addressed:
Collection transition verified: Lines 695-699 correctly return
ПараметрыМетодаБенчмаркаas enumerable collection with.Количество()and.Добавить()methods.КомбинаторПараметровБенчмарка support confirmed: Handles the type via explicit check (
Тип("ПараметрыМетодаБенчмарка")at line 38, 45) and properly converts using.ВМассив()method.Parameter output support confirmed:
ВывестиПараметры(lines 963-974) iterates usingДля Каждогоloop, which works becauseПараметрыМетодаБенчмаркаhas the&ДляКаждогоannotation enabling enumeration.The class is fully enumerable and compatible with all consuming code.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/BenchmarkOneScript/cmd/Классы/КомандаЗапуска.os`:
- Around line 222-227: The CLI exporter parsing splits _Экспортеры using ", "
which breaks documented input like "-e json,html"; change the split to use ","
and trim whitespace from each resulting element before lookup: call
СтрРазделить(_Экспортеры, ",") then for each element apply СтрОбрезать (or
equivalent) before computing Имя = ДопустимыеЗначения[НРег(Экспортер)] and
asserting Ожидаем.Что(...).Заполнено(); this ensures comma-only input and inputs
with incidental spaces both match.
In `@src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os`:
- Around line 1186-1200: В процедуре names опечатка: переименуйте процедуры
ДессериализоватьОбработчики → ДесериализоватьОбработчики и
ДессериализоватьЭкспортеры → ДесериализоватьЭкспортеры; обновите все вызовы и
ссылки на эти процедуры в проекте (поиск по имени процедуры), включая места, где
они экспортируются или используются совместно с ДобавитьОбработчикСобытия и
ДобавитьЭкспортер, и проверьте, что сигнатуры и видимость остаются прежними,
чтобы не нарушить совместимость.
In `@src/BenchmarkOneScript/core/Классы/РезультатыБенчмарков.os`:
- Line 151: The procedure name contains a typo: rename the procedure
ПереопределитьДескрпиторыВЗапусках to ПереопределитьДескрипторыВЗапусках and
update every call/reference to it (including the caller that currently invokes
ПереопределитьДескрпиторыВЗапусках) so identifiers match; ensure the procedure
declaration and all usages (calls, event bindings, references) are consistently
renamed.
In `@src/BenchmarkOneScript/core/Классы/СериализаторРезультатовБенчмарков.os`:
- Around line 26-31: В методах ЗаписатьВJson и ПрочитатьИзJson оберните работу с
потоками (объекты ЗаписьТекста и ЧтениеТекста) в блок Попытка/Исключение или
Попытка/Исключение/Вычистить: внутри Попытка выполняйте
Сериализатор.Сериализовать и Записать(…) / ЧтениеТекста.Прочитать(), в блоке
ВЫЧИСТИТЬ/НАВСЕГДА (или в блоке Иначе после Исключение) гарантированно вызывайте
Закрыть() для соответствующего потока, а при перехвате ошибки повторно
выбрасывайте исключение, чтобы не подавлять его; используйте имена методов
ЗаписатьВJson, ПрочитатьИзJson и объектов ЗаписьТекста/ЧтениеТекста для
локализации изменений.
In `@src/BenchmarkOneScript/core/Классы/СтатистикаБенчмарка.os`:
- Around line 296-312: ПолучитьПроцентили() currently passes raw percentile
labels (e.g., 0,25,...,100) into Квантиль; normalize the probability to the 0–1
range before calling Квантиль by dividing the parsed percentile value by 100
(e.g., compute Вероятность = Число(Сред(Строка.Ключ, 2)) / 100) and then call
Квантиль(Вероятность) for each entry in the Процентили structure (ensure P0→0
and P100→1 are handled correctly).
In `@src/BenchmarkOneScript/custom-jason/Классы/ДесериализаторJsonBench.os`:
- Around line 173-181: The code calls ДесериализуемыеКлючи.Свойство(ИмяКолонки)
without ensuring ДесериализуемыеКлючи is defined; update the loop in the
deserialization routine (the block that iterates ЗначенияКолонок and assigns
ИмяКолонки/ЗначениеКолонки) to first check that ДесериализуемыеКлючи is not
Неопределено (and is an object) before invoking its Свойство method, and only
remap ИмяКолонки when that check passes.
In `@src/BenchmarkOneScript/dto/Классы/РезультатЗапускаБенчмаркаДто.os`:
- Around line 89-121: В процедуре ДесериализоватьПараметры найдено мёртвое
состояние: массив ПараметрыМетода создаётся и заполняется но никогда не
используется; либо передавайте его в ДескрипторБенчмарка, либо полностью
убирайте связанный код. Исправьте одно из двух: 1) если ДескрипторБенчмарка
должен знать порядок параметров — добавить метод/вызов (например
УстановитьПараметрыМетода или аналог) и передать ПараметрыМетода после цикла;
либо 2) удалить объявление ПараметрыМетода, создание с новым размером и
присвоения ПараметрыМетода[ИндексПараметра] внутри цикла, оставив только
заполнение коллекции Параметры (и при необходимости корректировать логику
определения ЭтоПараметрМетода). Уточните изменения в коде рядом с именами
ДесериализоватьПараметры, ПараметрыМетода, ДескрипторБенчмарка и Параметры.
🧹 Nitpick comments (9)
src/BenchmarkOneScript/dto/Классы/РезультатЗапускаБенчмаркаДто.os (1)
98-102: Consider simplifying array initialization.The conditional check for
КоличествоПараметров = 0is unnecessary sinceНовый Массив()andНовый Массив(0)produce equivalent results. However, if the intent is to pre-allocate the array with a specific capacity, verify thatНовый Массив(КоличествоПараметров)actually pre-sizes the array in OneScript.♻️ Suggested simplification
- Если КоличествоПараметров = 0 Тогда - ПараметрыМетода = Новый Массив(); - Иначе - ПараметрыМетода = Новый Массив(КоличествоПараметров); - КонецЕсли; + ПараметрыМетода = Новый Массив(КоличествоПараметров);src/BenchmarkOneScript/core/Классы/РезультатыБенчмарков.os (3)
72-72: Duplicate region declaration.There are two
#Область СлужебныеПроцедурыИФункцииsections (lines 72 and 140). Consider merging them into a single region or renaming the second one (e.g.,СлужебныеПроцедурыИФункцииДесериализации).Also applies to: 140-140
74-107: Missing explicit return value when etalon is not found.
НайтиЭталонДляСтрокиreturns implicitly when no match is found. While OneScript returnsНеопределеноby default, an explicitВозврат Неопределено;after the loop improves clarity and documents the expected behavior.Suggested improvement
КонецЦикла; + Возврат Неопределено; + КонецФункции
82-82: Variable shadows function name.Local variable
ПараметрыСовпадаютshadows the functionПараметрыСовпадают(...)defined below at line 109. This can cause confusion during maintenance. Consider renaming the variable (e.g.,ПараметрыСовпалиorФлагПараметрыСовпадают).src/BenchmarkOneScript/core/Классы/ЭкспортерРезультатовБенчмарковВHtml.os (1)
15-18: Consider validating ПолноеИмяФайла before writing.Since the path is now caller-supplied, a simple guard (non-empty path and/or clear error if the directory doesn’t exist) would make failures easier to diagnose.
Also applies to: 66-70
src/BenchmarkOneScript/core/Классы/ДескрипторБенчмарка.os (2)
276-282: Potential NPE if_Типis invalid after deserialization.In
ПослеДесериализации,Новый(_Тип)is called without checking if_Типis a valid, instantiable type. If deserialization fails to restore the type correctly, or the type requires constructor arguments, this will throw an unexpected exception.Consider adding a guard or wrapping in try-catch with a more descriptive error:
Proposed improvement
&ПослеДесериализации Процедура ПослеДесериализации() Экспорт Валидировать(); + Попытка ИзвлечьИменаПараметров(Новый(_Тип)); + Исключение + ВызватьИсключение СтрШаблон("Не удалось создать экземпляр типа <%1> для извлечения имен параметров: %2", _Тип, ОписаниеОшибки()); + КонецПопытки; КонецПроцедуры
284-293: Consider makingВалидировать()exported for external validation needs.
Валидировать()is currently a private procedure but may be useful for external callers who want to validate a descriptor without going through deserialization. This would align with the exportedВалидировать()pattern seen inКонфигурацияБенчмарков.os.src/BenchmarkOneScript/custom-jason/Классы/СериализаторJsonBench.os (1)
188-193: Consider throwing an exception for unknown types instead of returningНеопределено.When an unknown type is encountered, the current implementation logs a warning and returns
Неопределено. This can lead to silent data corruption in the serialized output, making deserialization issues hard to diagnose.Proposed improvement
Если НЕ ЭтоПользовательскийТип Тогда - Лог.Предупреждение("Неизвестный тип объекта для преобразования: %1", ТипОбъекта); - Возврат Неопределено; + ВызватьИсключение СтрШаблон("Неизвестный тип объекта для преобразования: %1. Добавьте конвертер или пометьте тип как сериализуемый.", ТипОбъекта); КонецЕсли;src/BenchmarkOneScript/core/Классы/ЗапускательБенчмарков.os (1)
1023-1031: Consider scope of exported factory function.
НовыеРезультатыБенчмарковis marked asЭкспортbut appears to be an internal helper that initializes results with private instance variables (_ДескрипторыБенчмарков,_Конфигурация). If this is only used internally, consider removing theЭкспортmodifier to reduce the public API surface.
src/BenchmarkOneScript/core/Классы/СериализаторРезультатовБенчмарков.os
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os (1)
963-975: Булево поле “Модифицирован” лучше заполнять значением по умолчанию.
Сейчас в таблицу может попастьНеопределено, что не всегда совместимо с типомБулево.🛠️ Возможное исправление
Для Каждого ОписаниеПоля Из ОписанияПолей() Цикл СтрокаТаблицы = Таблица.Добавить(); СтрокаТаблицы.Имя = ОписаниеПоля.Имя; СтрокаТаблицы.Значение = ОписаниеПоля.Геттер.Исполнить(); - СтрокаТаблицы.Модифицирован = _МодифицированныеПоля[ОписаниеПоля.Имя]; + ЗначениеМодифицировано = _МодифицированныеПоля[ОписаниеПоля.Имя]; + Если ЗначениеМодифицировано = Неопределено Тогда + ЗначениеМодифицировано = Ложь; + КонецЕсли; + СтрокаТаблицы.Модифицирован = ЗначениеМодифицировано; КонецЦикла;
🤖 Fix all issues with AI agents
In `@packagedef`:
- Around line 21-37: Update the dependency versions for annotations and lambdas:
change the .ЗависитОт("annotations", "1.3.1") entry to .ЗависитОт("annotations",
"1.4.0") and change .ЗависитОт("lambdas", "0.3.2") to .ЗависитОт("lambdas",
"0.3.3") so the packagedef reflects the package hub's current versions while
leaving all other .ЗависитОт(...) entries unchanged.
In `@src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os`:
- Around line 1130-1146: В процедуре ПослеДесериализации(ПрочитанныеСвойства)
инициализируй поля `_Рефлектор`, `_ЗначенияПолейПоУмолчанию`,
`_МодифицированныеПоля`, `_Обработчики` и `_Экспортеры` в безопасные значения
если они = Неопределено перед вызовами
ДесериализоватьОбработчики/ДесериализоватьЭкспортеры и до вызова
ОпределитьМодифицированныеПоля(), чтобы избежать обращения к Неопределено;
конкретно — в начале ПослеДесериализации проверить каждое из этих полей и
присвоить объект/пустой массив/пустой справочник по умолчанию, затем продолжить
с существующими вызовами (см. имена процедур ПослеДесериализации,
ОпределитьМодифицированныеПоля, ДесериализоватьОбработчики,
ДесериализоватьЭкспортеры).
- Around line 1186-1200: В процедурах ДесериализоватьОбработчики и
ДесериализоватьЭкспортеры добавьте проверку наличия соответствующих ключей в
ПрочитанныеСвойства перед итерацией: вызовите
ПрочитанныеСвойства.Свойство("Handlers") и
ПрочитанныеСвойства.Свойство("Exporters") и только если true — выполнять цикл,
иначе пропустить; это защитит вызовы ДобавитьОбработчикСобытия(Строка["Event"],
Строка["Handler"]) и ДобавитьЭкспортер(Новый(СтрокаТип)) от ошибки при
отсутствии секций.
In `@src/BenchmarkOneScript/core/Классы/СтатистикаБенчмарка.os`:
- Around line 377-386: ВыгрузитьЗамеры сейчас возвращает пустой массив когда
мониторинг памяти выключен, из‑за чего вызов
МатематическиеФункцииБенчмарков.Среднее над пустым массивом присваивает
ВыделяемаяПамять = 0 и смешивает «нет измерений» с «0 байт». Измените логику
потребителя (где вычисляется ВыделяемаяПамять) или саму функцию ВыгрузитьЗамеры
так, чтобы при отсутствии измерений (ТребуетсяМониторингПамяти() = Ложь или
когда возвращаемый массив пуст) возвращалось Неопределено вместо пустого
массива; затем при назначении ВыделяемаяПамять проверяйте на Неопределено и
только в противном случае вызывaйте
МатематическиеФункцииБенчмарков.Среднее(_ЗамерыПамяти) — см. функции/переменные
ВыгрузитьЗамеры, ТребуетсяМониторингПамяти,
МатематическиеФункцииБенчмарков.Среднее и переменную ВыделяемаяПамять для
внесения правок.
In `@src/BenchmarkOneScript/custom-jason/Классы/ДесериализаторJsonBench.os`:
- Around line 25-27: В комментарии над объявлением переменной "Перем
Конвертеры;" исправьте опечатку слова "Прользовательские" на правильное
написание "Пользовательские" в строке "// Соответствие - Прользовательские
конвертеры для типа" — просто отредактируйте текст комментария, оставив
остальной код (включая переменную Перем Конвертеры) без изменений.
In `@src/BenchmarkOneScript/dto/Классы/РезультатЗапускаБенчмаркаДто.os`:
- Around line 89-109: В процедуре ДесериализоватьПараметры добавь защитную
проверку и сброс состояния: перед использованием очисти или заново инициализируй
коллекцию Параметры чтобы предотвратить накопление при повторном вызове; при
доступе к ПрочитанныеСвойства проверяй наличие ключей "Category" и "Baseline" и
вызывай ДескрипторБенчмарка.УстановитьКатегорию/ИспользоватьКакЭталон только
если соответствующие значения не пусты (или подставляй безопасные значения по
умолчанию); до итерации по ПрочитанныеСвойства["Parameters"] проверяй, что этот
элемент существует и является коллекцией, и только затем выполняй цикл и
создание ПараметрБенчмарка; сохрани текущую логику вычисления
ИндексПараметра/ЭтоПараметрМетода (используя ИменаПараметровБенчмарка) но
учитывай возможный Неопределено результат Найти.
🧹 Nitpick comments (2)
src/BenchmarkOneScript/custom-jason/Классы/ДесериализаторJsonBench.os (2)
119-123: Добавьте явный возврат для неизвестного типа.При неизвестном типе функция логирует предупреждение, но не возвращает значение явно, что приводит к неявному
Неопределено. Рекомендуется добавить явныйВозврат.Иначе Лог.Предупреждение("Неизвестный тип объекта: %1", ТипОбъекта); + Возврат Неопределено; КонецЕсли;
312-316: Рассмотрите логирование при некорректных аннотациях.Аннотации
СериализуемыйКлючс одним или менее параметрами молча игнорируются. Для упрощения отладки конфигурации можно добавить предупреждение.КоличествоПараметров = Аннотация.Параметры.Количество(); Если КоличествоПараметров > 1 Тогда ДесериализуемыеКлючи.Вставить(Аннотация.Параметры[1].Значение, Аннотация.Параметры[0].Значение); + Иначе + Лог.Предупреждение("Аннотация СериализуемыйКлюч требует 2 параметра, получено: %1", КоличествоПараметров); КонецЕсли;
src/BenchmarkOneScript/custom-jason/Классы/ДесериализаторJsonBench.os
Outdated
Show resolved
Hide resolved
src/BenchmarkOneScript/dto/Классы/РезультатЗапускаБенчмаркаДто.os
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/BenchmarkOneScript/custom-jason/Классы/ДесериализаторJsonBench.os`:
- Around line 119-123: В блоке, где при неизвестном ТипОбъекта вызывается
Лог.Предупреждение, функция сейчас неявно возвращает Неопределено — замените
неявный возврат на явный: либо добавить явный Возврат Неопределено с понятным
сообщением, либо бросить исключение (Ошибка или Исключение) с текстом,
включающим ТипОбъекта, чтобы вызывающий код однозначно получал сигнал об ошибке;
обновите тело рядом с существующим Лог.Предупреждение так, чтобы сразу после
него выполнялся либо Возврат <значение> либо Бросить Исключение("Неизвестный тип
объекта: " + ТипОбъекта).
- Around line 333-339: Метод вызывается с пустым массивом аргументов для
случаев, когда СвойстваМетода.КоличествоПараметров > 1; исправьте формирование
ПараметрыМетода так, чтобы его длина соответствовала
СвойстваМетода.КоличествоПараметров — положите ПрочитанныеСвойства в позицию(и)
где ожидается объект (обычно первый параметр), а для остальных параметров
добавьте ЗначениеПоУмолчанию (Null) или соответствующие значения по контексту,
затем передайте этот массив в Рефлектор.ВызватьМетод(ПользовательскийОбъект,
СвойстваМетода.Имя, ПараметрыМетода).
🧹 Nitpick comments (2)
src/BenchmarkOneScript/custom-jason/Классы/ДесериализаторJsonBench.os (2)
157-159: ВозвратНеопределенодля пустого массива может быть неочевидным.При пустом входном массиве возвращается
Неопределеновместо пустойТаблицаЗначений. Это может вызвать NPE в вызывающем коде, ожидающем таблицу.💡 Альтернативный вариант
Если Объект.Количество() = 0 Тогда - Возврат Неопределено; + Возврат Новый ТаблицаЗначений; КонецЕсли;
247-250: Несоответствие типа логируется, но свойство молча пропускается.При несовпадении типа свойства данные теряются без явного уведомления вызывающего кода. Рассмотрите возможность выброса исключения или накопления ошибок для последующей обработки.
…ка через ЗаписьТекста.
…enchmarkOneScript into refactor/config-serialization
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/BenchmarkOneScript/core/Классы/КонфигурацияБенчмарков.os`:
- Around line 1198-1208: В процедуре ДесериализоватьЭкспортеры текущая строка
Создания экземпляра Новый(СтрокаТип) принимает любые имена из JSON — замените
это на проверку по whitelist: перед вызовом ДобавитьЭкспортер валидируйте
СтрокаТип против списка разрешённых имён экспортёров (например массив или
функция IsAllowedExporter), пропускайте или логируйте и выбрасывайте ошибку для
неподдерживаемых/вредоносных имён; используйте ДобавитьЭкспортер только для
прошедших валидацию значений и централизуйте whitelist рядом с кодом (или в
конфигурации) для простого управления.
In `@src/BenchmarkOneScript/core/Модули/Компаратор.os`:
- Around line 51-60: In function СоответствияСовпадают, add an explicit
key-existence check on Соответствие2 before accessing
Соответствие2[КлючЗначение.Ключ]; use Соответствие2.СодержитКлюч(Ключ) (or
СодержитКлюч(КлючЗначение.Ключ)) and return Ложь immediately if the key is
missing, then proceed to call Совпадают(Соответствие1[Ключ],
Соответствие2[Ключ]) only when the key exists.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/BenchmarkOneScript/core/Классы/РезультатыБенчмарков.os`:
- Around line 140-147: ПослеДесериализации не восстанавливает поле Отчет, потому
что оно помечено &Несериализуемое и конструктор при десериализации не
вызывается; добавьте инициализацию Отчет в процедуру ПослеДесериализации
(например вызовом новой или существующей функции инициализации отчёта) прямо
перед или после ПереопределитьДескрипторыВЗапусках(), чтобы гарантировать, что
Отчет не останется Неопределено; используйте имена из диффа
(ПослеДесериализации, Отчет, ПереопределитьДескрипторыВЗапусках,
ОпределитьЭталоны, ОбновитьСтатистику) чтобы найти место и восстановить
состояние Отчета аналогично тому, как это делает конструктор.
🧹 Nitpick comments (1)
src/BenchmarkOneScript/core/Классы/РезультатыБенчмарков.os (1)
82-82: Variable name shadows function name.The variable
ПараметрыСовпадаютshadows the functionПараметрыСовпадаютdefined at line 109. While this works in OneScript, it reduces code clarity. Consider renaming toПараметрыСовпалиorПараметрыИдентичны.Proposed fix
- ПараметрыСовпадают = ПараметрыСовпадают(РезультатЗапуска, ВходнаяСтрока); + ПараметрыСовпали = ПараметрыСовпадают(РезультатЗапуска, ВходнаяСтрока);Also update line 98:
- И ПараметрыСовпадают + И ПараметрыСовпали
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BenchmarkOneScript/core/Классы/ВалидаторЗапускаБенчмарков.os (1)
154-162: Update error message to reflect the actual primitive-only constraint.Line 154-155: The error message claims parameters must be "сериализуемым" (serializable), but the validation at line 157 only accepts primitives. Either broaden the validation to accept all serializable types (if intended) or update the message to match the primitive-only constraint.
✅ Recommended fix — align message with primitive-only validation
- "Значение параметра <%1> должно быть сериализуемым для определения эталонов между разными версиями OneScript"; + "Значение параметра <%1> должно быть примитивным для определения эталонов между разными версиями OneScript";The test
Тест_ПроверяетОшибкуЗапускаПериНаличииНесериализуемыхПараметровconfirms this is the intended behavior: it passes a Структура and expects rejection. The primitive-only design is intentional; only the message needs correction.
🤖 Fix all issues with AI agents
In `@src/BenchmarkOneScript/core/Классы/ВалидаторЗапускаБенчмарков.os`:
- Around line 175-179: The validation loop over
_Конфигурация.ОбработчикиСобытий() currently rejects any non-string handler
(ОбработчикСобытия.Обработчик) but the error message (ТекстОшибки added to
_СписокОшибок) says delegates aren't supported; either update the type check to
allow delegates by changing the condition to permit Тип("Делегат") (i.e., accept
ТипЗнч(...) = "Строка" OR = "Делегат") or keep the stricter string-only check
and change the error text (ТекстОшибки) to "обработчик должен быть строкой" so
the message matches the actual validation; apply this change in the loop that
references ОбработчикСобытия and _СписокОшибок in the ВалидаторЗапускаБенчмарков
class.
In `@tests/ТестыЭкспортеров.os`:
- Around line 36-40: Перед сравнением нормализуйте переводы строк в прочитанном
тексте и эталоне: вызовите замену CRLF → LF (удалите '\r') на переменных Текст
и/или ТекстЭталон перед передачей в Ожидаем.Что(...).Равно(...), например
применив метод замены строк на ЧтениеТекста/Текст/ТекстЭталон; обновите
аналогично сравнения в JSON/HTML тестах, чтобы сравнение через
Ожидаем.Что(…).Равно(…) работало независимо от CRLF vs LF.
- Around line 6-12: The cleanup in Процедура ПослеЗапускаТеста uses
ФС.УдалитьФайлы(КаталогАртефактов) which can throw if the directory doesn't
exist; make this idempotent by either checking existence first with
ФС.Существует(КаталогАртефактов) or wrapping the delete call in a try/except
(естественно logging only unexpected errors) so that absence of the
directory/files is ignored and the procedure always succeeds; update Процедура
ПослеЗапускаТеста to use the existence check or safe-delete pattern around
ФС.УдалитьФайлы referencing the same КаталогАртефактов symbol.
|
Stivo182
changed the title
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.