-
Notifications
You must be signed in to change notification settings - Fork 319
Глейзер Роман #252
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
base: master
Are you sure you want to change the base?
Глейзер Роман #252
Conversation
| @@ -1,14 +1,16 @@ | |||
|
|
|||
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.
Лучше не включать в МР, т.к. изменения были сгененрированы IDE автоматически и не имеют прямого отношения к МР
cs/Markdown/Pipeline.cs
Outdated
|
|
||
| namespace Markdown; | ||
|
|
||
| public class Pipeline(BlockSegmenter blockSegmenter, InlineParser parser, HtmlRenderer htmlRenderer) |
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.
На данный момент нет необходимости выносить эту логику в отдельный класс, давай перенесем ее в Md.Render()
cs/Markdown/Parsing/InlineParser.cs
Outdated
| // Предполагается реализация с разделением кода парсинга на отдельные классы, работающие в следующем порядке: | ||
| // Класс, посимвольно читающий текст | ||
| // Класс, преобразующий текст в единицы разметки (текст, бэк слеш, одинарное подчеркивание итд) | ||
| // Класс, отвечающий за обработку экранирования в тексте после преобразования текста в единицы разметки |
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.
Экранирование должно обрабатываться до парсинга тегов
cs/Markdown/Parsing/InlineParser.cs
Outdated
| throw new NotImplementedException(); | ||
| } | ||
|
|
||
| // Предполагается реализация с разделением кода парсинга на отдельные классы, работающие в следующем порядке: |
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.
Разбиение парсера на 5 классов избыточно, каждый из предложенных классов будет содержать 1-2 метода что усложнит архитектуру без реальной поьзы, классы не будут являться самостоятельными сущностями, а будут лишь шагами алгоритма
Отдельные классы стоило бы выделить в случае переиспользования логики или разных "профилей" парсера (например, с конфигурируемыми настройками), в этом кейсе такой необходимости нет
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.
Возможно для упрощения логики парсера стоит вынести отдельный этап предварительной обработки текста которая подготавливает его к основному парсингу, заменяя "мешающие" элементы на временные маркеры
cs/Markdown/Blocks/IBlock.cs
Outdated
|
|
||
| public interface IBlock | ||
| { | ||
| // Далее планируется добавление двух наследников: HeadingBlock и ParagraphBlock |
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.
Нет необходимости в наследниках исходя из текущих требований, лучше добавить поле BlockType. Разница HeadingBlock и ParagraphBlock только в рендеринге, хранятся и обрабатываются данные одинаково. Наследники понадобились бы, например, в случае разных структур данных
cs/Markdown/Blocks/IBlock.cs
Outdated
|
|
||
| public IReadOnlyList<INode> Inlines { get; } | ||
|
|
||
| void SetInlines(IReadOnlyList<INode> inlines); |
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.
Какие преимущества перед сеттером? Точно ли нужен этот метод?
Посидел, подумал и не нашел ни одной причины, по которой бы использование SetInlines было бы лучше использования дефолтного сеттера
Мне кажется под заданные требования будет достаточно просто сеттера. В итоговой реализации поправлю этот момент
|
Заметила что для некоторых компонентов выделены отдельные папки, содержащие по одному файлу. Предлагаю убрать, т.к. они избыточны и усложняют навигацию |
29a056e to
39c6c27
Compare
d0d2003 to
0b0c12d
Compare
cs/Markdown/Md.cs
Outdated
|
|
||
| public class Md | ||
| { | ||
| public string Render(string text, BlockSegmenter segmenter, InlineParser parser, HtmlRenderer renderer) |
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.
Давай передавать зависимости в конструктор, а не в метод. Класс должен скрывать детали реализации. Передавать в метод не стоит, т.к. не получится инжектировать зависимости через di + это нарушает инкасуляцию: любой внешний код вызывающий этот метод будет знать какие зависимости передавать, как их конфигурировать и т.д.
cs/Markdown/Inlines/InlineSyntax.cs
Outdated
| return position + length < text.Length ? text[position + length] : Space; | ||
| } | ||
|
|
||
| public static bool IsWordChar(char ch) |
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.
Избыточный метод
cs/MarkdownTests/MarkdownTests.sln
Outdated
| @@ -0,0 +1,24 @@ | |||
| Microsoft Visual Studio Solution File, Format Version 12.00 | |||
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.
не нужно тесты выделять в солюшн, проекта достаточно
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.
| } | ||
| } | ||
| return sb.ToString(); | ||
| } |
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.
Метод нечитабельный, нужно разбить/сделать компактнее
cs/Markdown/Inlines/InlineParser.cs
Outdated
| return text? | ||
| .Replace(PlaceholderUnderscore, Underscore) | ||
| .Replace(PlaceholderBackslash, Escape) | ||
| .Replace(PlaceholderHash, '#'); |
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.
магический символ
cs/Markdown/HtmlRenderer.cs
Outdated
| return sb.ToString(); | ||
| } | ||
|
|
||
| private static string RenderInlines(IReadOnlyList<Node> inlines, BlockType context) |
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.
Вынесем вспомогательные методы для читабельности? Метод получился большой и есть блоки которые напрашиваются в отдельный метод
cs/Markdown/HtmlRenderer.cs
Outdated
| break; | ||
|
|
||
| case NodeType.Em: | ||
| sb.Append("<em>") |
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.
магические строки
cs/Markdown/HtmlRenderer.cs
Outdated
| switch (node.Type) | ||
| { | ||
| case NodeType.Text: | ||
| sb.Append(node.Text ?? string.Empty); |
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.
В случае пусто строки ничего не добавляет, но вызывает метод. Лучше сделать проверку на пустую строку
| [TestFixture] | ||
| public class MarkupLanguageProcessorTests | ||
| { | ||
| private static string RenderHtml(string input) |
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.
давай уберем эти вспомогательные методы, они не сокращают кол-во строк в тесте, зато сокращают читабельность: вместо того чтобы открыть тест и посмотреть что там происходит, приходится проваливаться в эти методы и смотреть что там внутри
| } | ||
|
|
||
| [Test] | ||
| public void ParseAndRender_EmPartOfWord_RendersAtStartMiddleEnd() |
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.
Нужно разделить на 3 отдельных теста
| [Test] | ||
| public void ParseAndRender_EmWholeSentenceFromSpec_ProducesPlainTextHtml() | ||
| { | ||
| var input = "Текст, <em>окруженный с двух сторон</em> одинарными символами подчерка, должен помещаться в HTML-тег <em>."; |
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.
здесь что проверяется?
хотел проверить, что строки, содержащие html теги, не меняются и выводятся как обычный текст в абзаце
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.
| } | ||
|
|
||
| [Test] | ||
| public void ParseAndRender_OpeningUnderscoreMustBeFollowedByNonSpace_NotOpening() |
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.
очень запутанные названия, давай упростим и если требуется добавим описания к тестам
кстати, почему parseandrender когда тестируется метод render?
| } | ||
|
|
||
| [Test] | ||
| public void ParseAndRender_StrongPartOfWord_RendersAtStartMiddleEnd() |
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.
нужно разделить на 3 теста (возможно тесткейсы)
| } | ||
|
|
||
| [Test] | ||
| public void ParseAndRender_DelimiterFollowedByPunctuation_CommaAfter() |
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.
эти 3 логичнее объединить в тест с тесткейсами
| "Но <em>внутри __одинарного__ не</em> работает. Конец\\\\</p>"; | ||
|
|
||
| Normalize(RenderHtml(Paragraph)).Should().Be(expected); | ||
| } |
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.
Можно добавить тесты на граничные случаи (например, пустые строки/только пробелы между разделителями/максимальная вложенность/unicode символы и др)
| [Test] | ||
| public void ParseAndRender_ParagraphFromSpec_AllRulesTogether_ProducesExpectedHtml() | ||
| { | ||
| const string Paragraph = |
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.
почему константа? почему с большой буквы? отличается от других тестов
| } | ||
|
|
||
| [Test] | ||
| public void ParseAndRender_ParagraphFromSpec_AllRulesTogether_ProducesExpectedHtml() |
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.
антипаттерн, если тест упадет то не понятно почему именно упал и что сломалось, нарушен принцип 1 тест = 1 сценарий
cs/MarkdownTests/PerformanceTests.cs
Outdated
| [TestFixture] | ||
| public class PerformanceTests | ||
| { | ||
| private const string Paragraph = |
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.
+ можно добавить несколько итераций для усреднения результатов теста
| namespace MarkdownTests; | ||
|
|
||
| [TestFixture] | ||
| public class PerformanceTests |
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.
Можно добавить тесты на конкретные случаи, например, очень большая вложенность


@masssha1308
upd:
Предполагаемый алгоритм обработки текста: