Заколюкин Степан #227
Заколюкин Степан #227
Conversation
ObjectPrinting/PrintingConfig.cs
Outdated
| throw new ArgumentException(); | ||
| var memberExpr = (MemberExpression)memberSelector.Body; | ||
| if (memberExpr.Member.MemberType != MemberTypes.Field && memberExpr.Member.MemberType != MemberTypes.Property) | ||
| throw new ArgumentException(); |
There was a problem hiding this comment.
Здесь бы в исключении написать в чем проблема селектора, который нам передали. В коде это очень просто подсветить, а нашей SDK будет сильно проще пользоваться
There was a problem hiding this comment.
Исправил. Добавил сообщение, поясняющее почему конкретно возникла ошибка
There was a problem hiding this comment.
- У этого исключения есть хорошая перегрузка: https://learn.microsoft.com/en-us/dotnet/api/system.argumentexception.-ctor?view=net-8.0#system-argumentexception-ctor(system-string-system-string)
Она красиво выедет, что не так с параметром, если ты передашь message
- Старайся для всех параметров использовать
nameof (memberSelector)-- тогда если другой разработчик (или ты) переименует аргумент, сообщение останется актуальным.
There was a problem hiding this comment.
Исправил, использовал для исключения указанную перегрузку и nameof()
|
|
||
| public PrintingConfig<TOwner> Using(CultureInfo? culture) | ||
| { | ||
| return AddAlternativeSerializationMethod(obj => |
There was a problem hiding this comment.
Посмотри на конвертор. Это более привычный и правильный вариант сериализации в dotnet
https://learn.microsoft.com/en-us/dotnet/api/system.convert.tostring?view=net-9.0
There was a problem hiding this comment.
Исправил. Поменял на Convert.ToString(obj, culture)
| } | ||
|
|
||
| [Test] | ||
| public void PrintToString_CyclicLinks_NoStackOverflowException() |
There was a problem hiding this comment.
У тебя есть тест, который отражает, что исключения не будет. Но нигде не зафиксировано, что произойдет. И мне кажется, что это очень важно, чтобы такие ситуации были понятными.
Т.е.
- Такие кейсы не будут понятны: нет исключения, нет какого-то вывода в сериализуемом файле
- Такой кейс не зафиксирован в тестах
There was a problem hiding this comment.
Исправил. Добавил тест, отражающий как себя будет вести сериализатор. Кроме того переписал логику класса сериализации, для более корректной обработки циклических ссылок
ObjectPrinting/PrintingConfig.cs
Outdated
| } | ||
|
|
||
| private string PrintToString(object obj, int nestingLevel) | ||
| private string PrintToString(object? obj, int nestingLevel, HashSet<object> searized) |
There was a problem hiding this comment.
Опечатка в названии аргумента) Должно быть serIALIzed
ObjectPrinting/PrintingConfig.cs
Outdated
| { | ||
| private static readonly ImmutableArray<Type> FinalTypes = |
There was a problem hiding this comment.
NIT: Эти поля не будут шариться между всеми классами. В dotnet так не работает :(
Если честно, я бы переписал так код, чтобы проверки были в отдельном методе. Смысла от общей структуры тут очень мало.
There was a problem hiding this comment.
Не исправил. Не совсем понял что ты имеешь ввиду, вообще убрать это поле и создать метод, в котором входной тип будет проверяться на перечисленные в FinalTypes или просто обернуть FinalTypes.Contains(inputType) в метод
There was a problem hiding this comment.
Оба варианта норм! Поступай на своё усмотрение
There was a problem hiding this comment.
Главное, чтобы у класса с generic параметрами не было статических полей :) Только в тех случаях, когда без этого никак
There was a problem hiding this comment.
Исправил, обернул проверку на финальный тип в метод IsFinalType(Type type)
|
|
||
| namespace ObjectPrinting.Tests; | ||
|
|
||
| public class PrintingConfigTests |
There was a problem hiding this comment.
Такое ощущение, что большинство тестов имеют очень маленький extected и его проще оставить в коде. Потому что:
- проще читать и расширять
- не может быть проблем с файлами
...
Но история с тем, что у тебя есть тесты из файлов очень крутая для функциональных тестах: их суть в том, чтобы за 1 тест проверить очень много логики на жирных данных. Такие тесты полезны, например тем, чтобы на уровне тестов давать гарантии по обратной совместимости: сгенерил вывод для оч большого обьекта, проверил его глазами и положил в файл. Этот тест начнёт падать, если какие-то данные сериализации начнут меняться (из-за обновления дотнета, кривого кода и т.п.)
There was a problem hiding this comment.
Исправил. Тесты с небольшим extected вытащил в код
There was a problem hiding this comment.
Раз ты вытащил в код, кажется, можно убрать кучу ненужных полей, а содержание не поменяется. Ну и ИМХО строки через @"" и переносы строк выглядит более лаконично.
There was a problem hiding this comment.
Ты имеешь ввиду убрать expected в строку с проверкой?
| CheckSerializationOfTheCollection(expected, dict); | ||
| } | ||
|
|
||
| private void CheckSerializationOfTheCollection<TCollection>(string expected, TCollection collection) |
There was a problem hiding this comment.
Тут не полностью выполнено требование: о перечислении всех публиных полей: у списка нет свойства Count.
There was a problem hiding this comment.
Поправил логику в классе сериализатора и тесты
| return sb.ToString(); | ||
| } | ||
|
|
||
| private string SerializeNameOfType(Type type) |
There was a problem hiding this comment.
NIT: тот формат, как пишет Type.GetType().ToString() более распространённый формат. Всякие декомпиляторы часто его использую и он общепризнанный. А то, что он не совпадает с тем, что пишем мы в IDE в нормально. Не все, что мы пишем в коде, легко конвертируется в код и обратно!
There was a problem hiding this comment.
Я решил использовать такой способ, чтобы было понятнее какой тип имеют объекты при чтении сериализации, лучше поменять?
No description provided.