Зубков Артём #260
Зубков Артём #260
Conversation
Yrwlcm
left a comment
Yrwlcm
left a comment
There was a problem hiding this comment.
Круто что знаешь/почитал про это и нашел сразу правильное решение, респект 😎 Парочку комментов оставил, в остальном все гуд, можешь писать код
cs/Markdown/Tokenizer/Token.cs
Outdated
| public TokenType Type { get; } | ||
| public int Position { get; } | ||
|
|
||
| public Token(string value, TokenType type, int length, int position) |
There was a problem hiding this comment.
Потом бы наверное сам заметил, но тут в конструкторе длину получаем, которую мы никуда не сетим
cs/Markdown/Tokenizer/TokenType.cs
Outdated
| StrongEnd, | ||
| EmphasisStart, | ||
| EmphasisEnd, | ||
| BackSlash, |
There was a problem hiding this comment.
Предлагаю чуть поменять нейминг у BackSlash. Такие символы обычно называются Escape character, пускай это будет Escape токен
| @@ -0,0 +1,13 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public enum TokenType | |||
There was a problem hiding this comment.
И еще я бы предложил еще выделить пару видов токенов, а может еще как-то у тебя получится решить эту проблему. Потому что сейчас кажется со сценарием ниже будут трудности, если токенайзер плохо отработает
Однако выделять часть слова они могут: и в _нач_але, и в сер_еди_не, и в кон_це._
В то же время выделение в ра_зных сл_овах не работает.
There was a problem hiding this comment.
В целом, сделано неплохо, но слишком много недочетов нашлось
И еще я запихал полностью текст спецификации маркдауна отсюда
https://raw.githubusercontent.com/artem7841/clean-code/refs/heads/master/MarkdownSpec.md
И там на половине текста отвалился парсинг заголовков. И в одном месте не сработало экранирование - тоже минус.
Жалко, что вам маркдаун перенесли раньше, чем вторую лекцию по тестированию. Т.к. в маркдауне очень помог бы апрувал тест, который раньше перед ним проходили. Это в общем тест, который грубо говоря сверяет файлики, и если ничего не изменилось - зеленый. А если изменился - то красный и показывает тебе разницу, что изменилось.
Варианта 2, ты можешь забежать вперед и попробовать сам запилить апрувал тест на полный текст спецификации. Либо можешь через raw строку в c# (""") прямо запихать текст в тест полностью. Чтобы его валидировать
А еще ты вроде через райдер пишешь, начни обращать внимания на всякие подчеркивания, которые он делает - они не просто так и часто помогают сделать код красивее, читабельнее или безопаснее
| { | ||
| private Md md; | ||
|
|
||
| [SetUp] |
There was a problem hiding this comment.
Вы вроде пока еще не проходили тему с DI и это очень отдаленно с ней связано. Нам ведь не обязательно на каждый тест инициализировать Md и можно поискать атрибут в NUnit-е, который позволяет сетапится один раз перед всеми тестами
В других файлах с тестами тоже
| private Tokenizer tokenizer = new Tokenizer(); | ||
| private Parser parser = new Parser(); | ||
| private Renderer renderer = new Renderer(); |
There was a problem hiding this comment.
У меня есть пара мыслей, но чтобы тебя к ним подвести, хочу спросить, а почему решил так сделать, а например не в методе инициализировать?
| [TestCase("Текст - не пункт", "Текст - не пункт")] | ||
| [TestCase("-пункт без пробела", "-пункт без пробела")] | ||
| [TestCase("# Заголовок\n- пункт\n- еще пункт", "<h1>Заголовок</h1><ul><li>пункт</li><li>еще пункт</li></ul>")] | ||
| [TestCase("- пункт\n# Заголовок\n- еще пункт", "<ul><li>пункт</li></ul><h1>Заголовок</h1><ul><li>еще пункт</li></ul>")] |
There was a problem hiding this comment.
Тут немного не по спецификации сделано, потерялись все отступы после рендера. Из-за того что это разметка, может показаться, что не критично. Но вот условно лазить по строке подряд идущих 15 элементов списка я не хочу)
| [TestCase("текст\\", "текст\\")] | ||
| [TestCase("сим\\волы", "сим\\волы")] |
There was a problem hiding this comment.
Последние два теста разве не экранирование проверяют?
|
|
||
| [TestCase("__жирный__", "<strong>жирный</strong>")] | ||
| [TestCase("Обычный _курсив_ и __жирный__ текст", "Обычный <em>курсив</em> и <strong>жирный</strong> текст")] | ||
| public void Render_StrongTag_HandlesCorrectly(string input, string expected) |
There was a problem hiding this comment.
Проверяем рендер жирного текста, а в тесте фигурирует курсив. При этом особые случаи по типу в разных словах и только начальные/конечные подчерки мы не проверили, хотя это спецификация и для курсива и для жирного текста
P.s. подумай их не дублировать
|
|
||
| private Node ParseHeader() | ||
| { | ||
| var start = position; |
| position++; | ||
|
|
||
| var header = new HeaderNode(); | ||
| while (position < tokens.Count && tokens[position].Type != TokenType.NextLine) |
There was a problem hiding this comment.
Кажется, что можно отказаться от части проверок position < tokens.Count, если начать пользоваться тем, что ParseToken() возвращает нулл если прочитал все токены. Ну и может это как-то покрасивее обыграть
| { | ||
| var start = position; | ||
|
|
||
| if (!TrySkipListItemPrefix()) { return new TextNode("-");} |
There was a problem hiding this comment.
Условие и тело условия должны быть на разных строчках
| return previousToken.Type == TokenType.Text; | ||
| } | ||
|
|
||
| private void ProcessEmphasisContent(EmphasisNode node, ref bool hasSpaceInside, bool inEmphasisContext) |
There was a problem hiding this comment.
А почему hasSpaceInside именно по ссылке передаем? А не аут параметром или возвращаем из метода
inEmphasisContext не используется
| private Node ParseToken(bool inEmphasisContext = false) | ||
| { | ||
| if (position >= tokens.Count) | ||
| return null; |
There was a problem hiding this comment.
Когда явно возвращаешь null в новых версиях шарпа лучше помечать возвращаемый тип нулабельным, т.е. вот так private Node? ParseToken(bool inEmphasisContext = false). Так статический анализатор кода будет больше и по делу давать подсказки по нулабельности. Там где-то еще в парсере может вернутся нулл в методе, там тоже стоит перевести на нулабельный тип
2 participants
No description provided.