Conversation
Yrwlcm
left a comment
Yrwlcm
left a comment
There was a problem hiding this comment.
Кажется если начать реализовывать текущее решение, то на этапе парсинга текста станет сильно больно. Свои мысли по решению написал. Но не исключаю, что я пропустил ключевую идею, из-за которой твое решение норм, так что я открыт к обсуждению :)
Предлагаю тебе пересмотреть решение и перейти на другой способ парсинга синтаксиса языков (который, вроде бы, и используется в основном) и поискать про него информацию
Там используется токенайзер, который текст превращает в набор токенов, с которыми будет удобнее работать и единственный парсер, который как раз эти токены читает и на их основе собирает как раз дерево из нод, которое ты уже используешь.
Так у тебя будет централизованная логика в парсере без дублирования и её проще будет менеджментить. А еще результат будет линейный от входа
cs/Markdown/MarkdownDocument.cs
Outdated
| public class MarkdownDocument | ||
| { | ||
| private DocumentNode root; | ||
| private MarkdownNode currentNode; | ||
|
|
||
| public MarkdownDocument() | ||
| { | ||
| root = new DocumentNode(null, ""); | ||
| } | ||
|
|
||
| public void AddNode(MarkdownNode node) | ||
| { | ||
|
|
||
| } | ||
|
|
||
| public string ToHtml() | ||
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
На самом деле не понял, зачем нам нужен этот класс и как он будет работать, есть только додумки. Мы хотим построить дерево, у которого потом вызовем .ToHtml() и пускай оно само нам строчку вернет - это правильно, так по идее и должно быть
Предполагаю у нас для этого есть DocumentNode и мы все ноды в него будем сворачивать. Нооо как раз нода документа - сейчас нигде не фигурирует в коде и кажется либо она, либо этот класс лишний
Еще не совсем понял как у нас в этом классе документа будет реализовано AddNode списков тут нет, а вызывать у чего-то дочернего по цепочке AddNode, кажется пока сомнительно, но может я не до конца понял что ты хочешь сделать
There was a problem hiding this comment.
Планировалась логика добавления нод, для этого в поле есть DocumentNode, но в принципе я согласен, класс излишен
| namespace Markdown.Nodes.Interfaces; | ||
|
|
||
| public abstract class InternalMarkdownNode : MarkdownNode | ||
| { | ||
| protected readonly List<MarkdownNode> children = []; | ||
| public override void Add(MarkdownNode node) | ||
| { | ||
|
|
||
| } | ||
|
|
||
| public override List<MarkdownNode> GetChildren() | ||
| => children; | ||
|
|
||
| protected InternalMarkdownNode(MarkdownNode? parent, string value) : base(parent, value) | ||
| { | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Этот класс сейчас тоже кажется избыточным. Вот мои доводы почему:
Мы хотим, чтобы наш класс MarkdownNode и все его реализации умели делать следующее:
Отдельно выделю, т.к. коммент не про это
- Хранить свое значение
- Знать своего родителя
- Добавлять в себя элемент
- Хранить весь список элементов
Будто бы сразу же приходит в голову список - ровно то, для чего он нужен. И тогда кажется логичным пускай сразу в MarkdownNode и реализуется этот список, для дерева это тоже подходит. А тут мы получается вынесли реализацию списка в отдельный класс и это прямо слишком абстракция
Сходу в голову не приходит адекватный сценарий, в котором мы решим не использовать список, а например словарь, потому что нам не нужны функции словаря, нам даже уметь доставать по индексу не нужно. А ради таких призрачных сценариев не стоит усложнять код, а просто следовать принципу KISS :)
There was a problem hiding this comment.
Нуу, в целом теперь окей, наверное если мы оставим реализацию того, что у нас листья наследуются от MarkdownNode то тогда и этот класс можно оставить с реализацией списка
| namespace Markdown.Nodes.Interfaces; | ||
|
|
||
| public abstract class LeafMarkdownNode : MarkdownNode | ||
| { | ||
| protected LeafMarkdownNode(MarkdownNode? parent, string value) : base(parent, value) | ||
| { | ||
| } | ||
|
|
||
| public override void Add(MarkdownNode node) | ||
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Тут кажется чтобы был правильный смысл, нужно поменять наследование. Мы ведь сейчас можем в листы дерева добавлять еще ноды, т.к. мне класс MarkdownNode это позволяет и я могу при желании к нему с апкастить. Пусть тогда у нас все наследуется от листа, где только значение, родитель и tohtml, а потом не листовые ноды с их списками
Ты еще занес в крайние ноды ImageNode, но что если я захочу написать например вот так
[my __bold__ image](src="http something ...")
There was a problem hiding this comment.
Мы не сможем добавлять в листы дерева (или если сделаем апкаст) еще ноды, т.к там будет эксепшн. В паттерне composite у листьев оставляют методы пустыми или выбрасывают эксепшн. Еще как вариант, сделать флаг, является ли нода листом.
Насчет поменять наследование, в теории должно сработать. И как ты говорил выше, можно отказаться от такого подхода вообще, оставив только список, и в данной задаче, как я понимаю, можно не усложнять?
There was a problem hiding this comment.
Вот эта часть мне не очень нравится в этом решении, хоть это и по паттерну
там будет эксепшн
Точнее, я не понимал в каком контексте мы будем этот метод .Add вызывать и каким способом мы будем понимать можем мы это сделать или нет. Если бы нам пришлось заворачивать что-то в трай кетч, это было бы плохо. Т.к. конкретно это исключение - часть ожидаемой логики, что у нас будут листья дерева. А не что-то непредвиденное
С учетом того, что переделали решение на токенайзер и парсер и теперь парсер должен сам понимать у кого что он может вызывать, чтобы собрать дерево - теперь такой подход норм и его можно оставить)
| using Markdown.Nodes.Interfaces; | ||
|
|
||
| namespace Markdown.Parsing.Interfaces; | ||
|
|
||
| public interface IParser | ||
| { | ||
| bool CanParse(string text); | ||
| MarkdownNode Parse(string text); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Идея с парсерами кажется плохой и если ты её начнешь реализовывать будет сильно больно:
- Это уже не линейная обработка входящего текста, у нас сложность рендера вырастет до
O(N * M), где N - длина выхода, M - кол-во парсеров.
Так как мы каждый символ должны будем прогнать через все наши парсеры, иначе что-то можем пропустить - Непонятно как выбрать парсер в случае
_курсива и__жирного текста - Как мы будем обрабатывать пересечения тегов? А вложенность? (а если и будем, то опять кажется каждый парсер должен знать про особенности других тегов, чтобы их обработать или нет)
- Сейчас в решении непонятно в какой момент из текста появляются конкретные ноды (типо Bold или Header и т.п.)
IParserэтого не подразумевает - Потерялась логика с экранированием символов, возможно она закладывалась в каждый парсер, но тогда это тоже сильное дублирование
cs/Markdown/MarkdownParser.cs
Outdated
| private BoldNode ParseStrong() | ||
| { | ||
| throw new NotImplementedException(); | ||
| } | ||
|
|
||
| private ItalicNode ParseEmphasis() |
There was a problem hiding this comment.
Придираюсь) Но кажется чутка странным, что мы вызываем ParseStrong а на выходе BoldNode, лучше одно название зафиксировать. Для ItalicNode ParseEmphasis тоже
| @@ -0,0 +1,16 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public enum TokenType | |||
There was a problem hiding this comment.
Закину этот пример на подумать, кажется сейчас с ним могут быть трудности при парсинге
Однако выделять часть слова они могут: и в _нач_але, и в сер_еди_не, и в кон_це._
В то же время выделение в ра_зных сл_овах не работает.
Yrwlcm
left a comment
Yrwlcm
left a comment
There was a problem hiding this comment.
Честно - я не стал смотреть реализацию парсеров и детально смотреть реализацию парсера. Это сложно читать и разбираться, как и что работает.
Одна из первых проблем этого решения - ParserContext, у нас относительно непредсказуемым образом он меняется то тут, то в парсерах. Каждый парсер еще и тип текущего токена может поменять в контексте, это прямо 100% нет, хотя бы потому, что мы точно можем этого избежать.
Если приходится менять тип текущего токена - значит токенайзер плохо выполнил свою работу. Добавь в него тогда нужную валидацию, а правда ли это токен или все же текст, посмотреть на соседние символы в токенайзере - норм. Считай что поток токенов к тебе приходит - иммутабельный, по нему можно только ходить, но не менять содержимое
Возможно ты еще не совсем понял, зачем нам вообще этот подход нужен, можешь еще поискать в интернете, но основная идея такая:
Мы можем внутри парсинга одних токенов, вызывать парсинг других. Ну т.е. вместо 2 огромных свичей, сначала в выборе парсера, потом в BuildMarkdownDocument и сложной логики запрятанной в каждом из парсеров. Будет один парсер с методом ParseNode().
В ней уже будет свич, или например методы TryReadHeaderNode, TryReadItalicNode и т.п. подряд, если какой-то выдал true и смог спарсить - окей, и идем дальше форычем по всем токенам, пока они не кончатся.
На примере метода заголовка, что он должен делать внутри
TryReadHeaderNode()
{
ReadHashToken()
ReadSpaceToken()
ParseNode()
ReadEndOfLine()
}
В момент вызова ParseNode мы рекурсивно зайдем в этот метод и считаем например все Italic и Bold внутри.
Если хочется понять, как обрабатывать то, что у нас в одном токене, не могут быть другие, как например в хедере не может быть еще хедер, можно в метод передавать ридонли лист с набором родителей, в которые он вложен, и проверять, что если у нас вызван TryReadHeader внутри уже хедера, то возвращать сразу False и т.д.
И еще не хватает отдельных тестов на токенайзер и парсер и желательно рендер, помимо тестов сразу на все. То, что все вместе работает - это хорошо, но мы никак не фиксируем логику конкретных частей, а значит мы не понимаем в какой момент и что мы сломали.
cs/Markdown/MarkdownTokenizer.cs
Outdated
| private int position; | ||
| private readonly string markdownText; | ||
| private char Prev => position - 1 < 0 ? ' ' : markdownText[position - 1]; | ||
| private char Next => position + 1 == markdownText.Length ? ' ' : markdownText[position + 1]; | ||
| public MarkdownTokenizer(string markdownText) | ||
| { | ||
| this.markdownText = markdownText; | ||
| } | ||
| public List<Token> Tokenize() |
There was a problem hiding this comment.
Тут все слиплось - приватные поля с конструктором и методом, добавь отступов
cs/Markdown/MarkdownTokenizer.cs
Outdated
|
|
||
| position++; | ||
| nextSymbol = Next; | ||
| if (prevSymbol != ' ' && nextSymbol != ' ' && nextSymbol != '_') |
There was a problem hiding this comment.
prevSymbol - вводящее в заблуждение название переменной. Не сразу понял, что это символ до нижних подчеркиваний в данном контексте, а не предыдущий символ. Хотел написать, зачем тут проверяем предыдущий символ, он ведь всегда _, если дошли до этого участка кода
Еще не нужны переменные которые дублируют свойства Next и Prev, зачем тогда эти свойства добавляли
cs/Markdown/MarkdownTokenizer.cs
Outdated
|
|
||
| } | ||
|
|
||
| private Token TokenizeImage() |
There was a problem hiding this comment.
Этот метод не нужен, т.к. все что он делает - это свич, а снаружи мы 5 раз его вызываем, чтобы получить конкретный результат свича
cs/Markdown/MarkdownTokenizer.cs
Outdated
| private bool IsSpecialSymbol(char symbol) | ||
| => specialsSymbols.Contains(symbol); | ||
|
|
||
| private readonly HashSet<char> specialsSymbols = ['#', '_', '!', '[', ']', '(', ')', ' ', '\n', '\\']; |
There was a problem hiding this comment.
А почему это положил в конец класса? И символы точно стоит вынести в общее место, вместе с теми что лежат внутри метода токенайзера. Туда добавил, сюда забыл, попробуй разберись почему не работает
cs/Markdown/ParseSelector.cs
Outdated
| case TokenType.LBracket: | ||
| case TokenType.LParenthesis: | ||
| case TokenType.RBracket: | ||
| case TokenType.RParenthesis: | ||
| case TokenType.Exclamation: |
There was a problem hiding this comment.
Кажется парсер не должен так работать, хотя бы с точки зрения того, что мы выбираем парсер на основе закрывающих символов и без какого-либо контекста
Т.е. наше решение развалится или превратится в еще большего монстра, если у нас появятся разные конструкции, которые используют одинаковый закрывающий символ, ну или разный контекст. В маркдауне конечно не факт, что такое встретится, но если посмотрим на решение - как подход к обработке формальных языков
cs/Markdown/Tests/MdTests.cs
Outdated
| [TestCase("", | ||
| "<img src =\"https://i.pinimg.com/originals/f5/ef/a6/f5efa6a__5b2__c76c038ef0c8d2502fd2f6.jpg\" alt=\"Cat\">", TestName = "UrlWithBold")] | ||
| [TestCase("![[][][]](https://i.pinimg.com/originals/f5/ef/a6/f5efa6a5b2c76c038ef0c8d2502fd2f6.jpg)", | ||
| "<img src =\"https://i.pinimg.com/originals/f5/ef/a6/f5efa6a5b2c76c038ef0c8d2502fd2f6.jpg\" alt=\"[][][]\">", TestName = "AltWithMultipleBrackets")] |
There was a problem hiding this comment.
Картинка котика это клево, но это жестко выглядит в коде и явной пользы за собой не несет для теста, переноси строки, когда они вылезают за экран. Горизонтальный скролл - плохо, и больно для ребят с ноутбуками, у которых такие строчки 2 экрана бы занимали
Это и тестов ниже касается и одной переменной, которая с текстом написана в одну длинную строчку
Если кратко, то в парсерах лежит логика валидации токенов (точно ли этот токен заголовок, а не текст и т.д), и их преобразование
Давай еще раз, в моем решении сейчас 3 этапа:
Правильно понимаю, что 1 и 2 этап должны слиться в один?
В ParseNode мы строим дерево, верно? Без свича же не будет O(n)? Точно форычем, как тогда смещать указатель на токен? |
cs/Markdown/Tests/MdTests.cs
Outdated
| [TestCase(4000, TestName = "Parse_4000_repeats")] | ||
| [TestCase(8000, TestName = "Parse_8000_repeats")] | ||
| [TestCase(16000, TestName = "Parse_16000_repeats")] | ||
| public void Render_Performance_WithAllTokens(int repeatCount) |
There was a problem hiding this comment.
Тест на производительность должен автоматически проверять, на сколько время увеличилось в зависимости от N(длины строки) или такого варианта достаточно?
There was a problem hiding this comment.
Идея теста правильная, реализация немного не та. Откажись от тесткейсов и сделай один тест, в котором также составляешь текст в N символов и в например 2*N символов. И потом внутри теста проверь, что первое число = второе число * 2, с учетом погрешности, разумеется
Да, мы не должны валидировать работу токенайзера. Он должен сразу работать правильно, обязанность одна, а разбили её на две составляющие
В целом да, только результирующие ноды ParseNode придется все равно сложить в какую-то родительскую ноду, потому что кажется не выйдет в ParseNode выдавать сразу полностью спаршенное дерево
Думаю получится и через свич реализовать в маркдауне, хотя не очень уверен. Но в целом да, я согласен что в худшем случае мы будем проходить какую-то часть токенов несколько раз. В общем случае, это наверное можно определить так, что в зависимости от того, сколько у нас будет конструкций языка, у которых префиксы совпадают, столько раз мы и будем повторятся по токенам. Ну условно Отличие от твоего первого решения в том, что мы не проверяем CanParse, мы сразу парсим и если не получилось - откатываемся. А значит не обязательно заходим в каждый парсер, чтобы точно убедится, что он не может спарсить
Неа, тут я сказал не то, о чем думал, скорее while-ом пока не индекс за пределы списка токенов не выйдет |
2 participants
@Yrwlcm
Идея следующая: посимвольно парсим, на каждый символ находим свой парсер в ParserSelector (думаю с помощью switch возвращать соответствующий символу парсер ), затем проверять, можно ли спарсить тег, если да, то парсим и получаем ноду, которую добавляем в древовидную структуру MarkdownDocument, если не парсится, то это текст, его так же добавляем в дерево. После того, как проитерируемся по всем элементам, у дерева вызовем метод ToHtml, он обойдет все элементы в глубину, вызовет у нод ToHtml и сформирует строку в формате html.
MarkdownDocument и ноды в папке Nodes - это паттерн композит. Он необходим, чтобы отслеживать взаимодействие и вложенность тегов.