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

Рыбин Леонид #229

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

Conversation

JavaScriptHaters
Copy link

No description provided.

using System.Text;

namespace ObjectPrinting
{
public class PrintingConfig<TOwner>
{
private readonly HashSet<Type> excludedTypes = new();
private readonly HashSet<MemberInfo> excludedProperties = new();
private readonly Dictionary<Type, Delegate> typeSerializers = new(); // ���������� �������

Choose a reason for hiding this comment

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

Сделай UTF8, пожалуйста, а не cp1251. Комментарии ломаются

Comment on lines 65 to 66
if (nestingLevel > MaxNestingLevel)
return "��������� �������� ������� ������������";

Choose a reason for hiding this comment

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

Если мы так начинаем обрабатывать циклические зависимости, то правда не упадет StackOverflowException, но для пользователей тоже не совсем очевидное поведение будет:
image

Copy link
Author

Choose a reason for hiding this comment

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

Добавил сеттер на максимальную длинну рекурсии

Comment on lines 55 to 56
Console.WriteLine("-----------");
Console.WriteLine(result);

Choose a reason for hiding this comment

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

Убрать


namespace ObjectPrinting;

public class PrintingConfiguration<TOwner, TPropType>

Choose a reason for hiding this comment

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

А что с названием? Чем PrintingConfig отличается от PrintingConfiguration по имени?

this PrintingConfiguration<TOwner, TNumericType> propertyStringConfiguration,
CultureInfo culture) where TNumericType : IFormattable
{
propertyStringConfiguration.ParentConfig.AddNumericCulture<TNumericType>(culture);

Choose a reason for hiding this comment

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

А почему только Numeric? У DateTime'а тоже есть Культура.
В условиях написано "Для всех типов, имеющих культуру, есть возможность ее указать"

Comment on lines 102 to 112
if (typeSerializers.TryGetValue(propertyType, out var typeSerializer))
{
sb.AppendLine($"{identation}{propertyName} = {typeSerializer.DynamicInvoke(propertyValue)}");
continue;
}

if (propertySerializers.TryGetValue(propertyName, out var propertySerializer))
{
sb.AppendLine($"{identation}{propertyName} = {propertySerializer.DynamicInvoke(propertyValue)}");
continue;
}

Choose a reason for hiding this comment

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

Копипаста

Copy link
Author

Choose a reason for hiding this comment

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

Опять же это можно сделать отдельной функцией, которая принимает сериалайзер, но она будет принимать очень много параметров, поэтому делать этого я не стал.

}

[Test]
public void PrintToString_PrintClassWithList_ShouldSerializeList()

Choose a reason for hiding this comment

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

Чет моделька для этого теста слишком перегружена:
есть и циклические зависимости, и в списке они странно сериализуются:
image

Copy link
Author

Choose a reason for hiding this comment

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

Несовсем понял проблемы. В стандартном тесте циклические зависимости не смотрятся, а список сериализуется как если бы его пришлось заполнять в коде
тест

}

[Test]
public void PrintToString_PrintClassWithDictionary_ShouldSerializeDictionary()

Choose a reason for hiding this comment

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

Словарь тож странно сериализуется, скобки почему-то в одной строке: закрывающая и открывающая
image

Choose a reason for hiding this comment

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

Моделька так же перегружена

Copy link
Author

Choose a reason for hiding this comment

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

Модель для всех тестов общая, так везде используется класс Person.

Copy link
Author

Choose a reason for hiding this comment

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

Добавил класс SimplePerson, чтобы в нём полей поменьше было


namespace ObjectPrinting.Tests
{
public class Person
public class Person//(Guid Id, string Name, double Height, int Age)

Choose a reason for hiding this comment

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

комментарий

namespace ObjectPrinting.Tests;

[TestFixture]
public class ObjectPrinterTests

Choose a reason for hiding this comment

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

Нету тестов пересекающихся:

  • Исключить строки, но добавить для них кастомную сериализацию. А наоборот?
  • Исключить из сериализации коллекции
  • Не увидел тестов с сериализацией времени
  • Несколько исключений подряд для разных типов
  • Несколько исключений для одинаковых типов
    и тд.

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