Skip to content
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

Мажирин Александр #231

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Kexogg
Copy link

@Kexogg Kexogg commented Dec 11, 2024

No description provided.

Copy link

@Buskervil Buskervil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Есть мелкие огрехи с дженерик параметрами (не критично)
  2. Нет поддержки полей

[SetUp]
public void Setup()
{
expectedString = File.ReadAllText($"ExpectedResults/{TestContext.CurrentContext.Test.MethodName}.txt");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сетап вызывается перед запуском каждого теста, да?
Он пишет строку в поле класса, которое используют все методы. Что будет, если запустить несколько тестов одновременно?

Повесь на класс атрибут [Parallelizable(ParallelScope.All)] и позапускай все тесты несколько раз (увидишь что они случайным образом падают). Сделай так чтобы работало с атрибутом тоже. Надо убрать сетап и доставать строку внутри тестов

}

[Test]
public void ObjectPrinter_Should_SerializeObject_WithExcludedNestedProperty()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

О, вот это интересно)

public string PrintToString(TOwner obj)
{
return PrintToString(obj, 0);
}

private string PrintToString(object obj, int nestingLevel)
protected internal static MemberInfo GetPropDetails<T>(Expression<Func<TOwner, T>> expression)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Наверное первый раз в живую вижу такой модификатор доступа)).
А почему не просто private?

var sb = new StringBuilder();
sb.Append('\t', nestingLevel);
sb.AppendLine(dictionary.GetType().Name);
foreach (DictionaryEntry entry in dictionary)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему не перебор ключей?

protected internal readonly Dictionary<MemberInfo, Func<object, string>>
AlternativeSerializationForProp = new();

protected internal readonly Dictionary<Type, Func<object, string>> AlternativeSerializationForType = new();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Указание культуры, обрезание строк можно было рассматривать как частный случай кастомного сериализатора по типу либо свойству/полю. (сейчас заменять не надо)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Там кстати есть экстеншн TrimmedToLength, который ты не используешь. Он как бы намекает, что это какой-то частный случай, который для удобства выписали отдельно.

private readonly Dictionary<Type, CultureInfo> cultureForType = new();
private readonly Dictionary<MemberInfo, CultureInfo> cultureForProp = new();
private readonly Dictionary<MemberInfo, int> trimLengthForProp = new();
private PropertyInfo? currentProp;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот эту штуку можно через методы потаскать, будет немного чище.

}

public IMemberPrintingConfig<TOwner, TFieldType> Serialize<TFieldType>(
Expression<Func<TOwner, object>> expression)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

вот тут надо согласовать тип параметра с типом извлекаемого свойства

@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
<TargetFramework>net9.0</TargetFramework>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Молодец, что вынес тесты в отдельную сборку (почти полностью). Хорошо бы еще в этом проекте лишние зависимости (от Nunit-а) почистить

public Person Parent { get; set; }
}

public class SkilledPerson : Person

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Код, который не пригодился, лучше удалять

return expression.Body switch
{
MemberExpression memberExpression => memberExpression.Member,
UnaryExpression { Operand: MemberExpression operand } => operand.Member,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не понимаю зачем здесь обрабатывать UnaryExpression. Как будто наоборот тут надо эксепшн кидать

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А, понял). Это интересно. У тебя в некоторых случаях примитивные типы заворачиваются в object, поэтому тут приходится делать странные вещи. Но кажется такой способ создает странные сайд эффекты. Например можно пытаться применять унарные операторы к ссылочным типам, а это в контексте сериализации вообще смысла никакого не имеет.

Короче). Было бы наверное правильнее (с точки зрения бизнес-логики этой библиотеки)
добавить дженерик параметр в методы типа такого
public PrintingConfig<TOwner> Exclude<TProp>(Expression<Func<TOwner, TProp>> expression)
чтобы не происходило каста к object-у и потом убрать обработку унарных выражений

Сейчас это можно не делать

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants