-
Notifications
You must be signed in to change notification settings - Fork 213
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
Овечкин Илья #190
base: master
Are you sure you want to change the base?
Овечкин Илья #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Очень странное название для проекта. Обычно, это что-то + .Tests.
} | ||
|
||
[Test] | ||
public void PrintToString_SerializesDictionary() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У меня этот тест валится)
|
||
namespace ObjectPrinting_Should | ||
{ | ||
[TestFixture] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тестов много, это хорошо. Но есть проблема - это не Acceptance тесты. Стоит сюда докинуть.
Плюс тесты очень жестко фиксируют текущий формат вывода. Малейшее изменение приведет к поломке всех тестов. Поэтому здесь и рекомендуется писать Acceptance тесты.
ObjectPrinting/PrintingConfig.cs
Outdated
if (configuration.typeSerialization.TryGetValue(propertyInfo.PropertyType, out var typeSerializer)) | ||
sb.Append(identation + propertyInfo.Name + " = " + typeSerializer.DynamicInvoke(propertyValue) + Environment.NewLine); | ||
else if (configuration.propertiesSerialization.TryGetValue(propertyInfo, out var propSerializer)) | ||
sb.Append(identation + propertyInfo.Name + " = " + propSerializer.DynamicInvoke(propertyValue) + Environment.NewLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А почему здесь Else if? Почему если мы применили одно, то не можем применить другое?
var finalTypes = new[] | ||
var type = obj.GetType(); | ||
|
||
if (finalTypes.Contains(type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В таком случае, получается, что если я принтер настрою на специфический формат инта, а вызову printer.PrintToString(123), то он просто напечатает 123, а не мой специфический формат.
return printingConfig; | ||
} | ||
|
||
public PrintingConfig<TOwner> Using(CultureInfo culture) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Reflection; | ||
using System.Globalization; | ||
using System.Linq.Expressions; | ||
using System.Collections; | ||
|
||
namespace ObjectPrinting | ||
{ | ||
public class PrintingConfig<TOwner> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не хватает иммутабельности.
ObjectPrinting/Config.cs
Outdated
|
||
namespace ObjectPrinting | ||
{ | ||
public class Config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Какое-то слишком абстрактное название. Конфигов в проекте может быть очень много, плюс некоторые фреймворки используют это названия для конфигов.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кажется, что нет понимания, что такое Acceptance-тесты)
Найди информацию на эту тему, почитай. И запили)
Это не сложно, написать такой тест - очень быстро. Одного будет более чем достаточно.
if (configuration.propertiesSerialization.TryGetValue(propertyInfo, out var propSerializer)) | ||
sb.Append(identation + propertyInfo.Name + " = " + propSerializer.DynamicInvoke(propertyValue) + Environment.NewLine); | ||
else | ||
sb.Append(identation + propertyInfo.Name + " = " + PrintToString(propertyValue, nestingLevel + 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь была речь о том, что мы, по логике, должны уметь применять к полю обе сериализации, как типа, так и самого поля.
В целом, конкретно это требование не указано в ТЗ, но кажется, что ограничение исходит из неоткуда.
Если это будет единственным замечанием итоговым, то я закрою на это глаза. Но лучше бы поправить)
using System.Reflection; | ||
using System.Globalization; | ||
using System.Linq.Expressions; | ||
using System.Collections; | ||
|
||
namespace ObjectPrinting | ||
{ | ||
public class PrintingConfig<TOwner> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DonielPankek