Смышляев Дмитрий #231
Смышляев Дмитрий #231
Conversation
- Также переименовал токенайзер
Засетапил архитектуру
- Сделал токенайзер - Поправил ошибки в сканерах
- Добавил тесты для сканера спец символов
- Довез тесы для текстового сканера - Поправил тесты для специального сканера
- Токены теперь не содержат строчку, но знают как ее найти - Поправил сканеры, с учетом новой логики
- И поправил тесты до кучи
- Добавил сканер для чисел - Добавил тесты для сканера чисел - Отрефакторил другие сканеры
- Теперь токен получает сорс текст, чтобы строить велью - Также появился дебаг ТуСтринг метод
- Довез тесты для токенизатора - Добавил токенизатору новый сканер
- Добавил ентер между актом и ассертом )
Готовый токенайзер
- Написал тулзы для упрощения работы с матчами - Накатил на это все тесты
- Добавил правило для примитивного италика
- Докинул ему добавление нескольких токен-типов - Добавил повторение токен-типов
- Переименовал токен TEXT на токен WORD
- Теперь не лист тайпов, а настоящий паттерн
- Добавил звезду Клини - Добавил первое совпадение
- Добавил метчу возможность билдить обратно лист
|
|
||
| namespace Markdown.Generator; | ||
|
|
||
| public class HTMLGenerator |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Согласен: сделаю, как начну генераторы пилить )
| @@ -0,0 +1,20 @@ | |||
| namespace Markdown.Tokenizer.Tokens; | |||
|
|
|||
| public class Token(TokenType tokenType, int begin, int length, string sourceText) | |||
There was a problem hiding this comment.
Это DataClass, в нем нет логики (совсем чуть-чуть :D), только данные.
Давай тогда заиспользуем Record?
There was a problem hiding this comment.
И для чего сюда передавать сразу весь текст sourceText?
Не говорю, что это плохо, просто мне не совсем понятно.
И еще на подумать, в aspnet есть такой концепт - Do not retrieve more data than is necessary. Он говорит про минимизацию запросов при http взаимодействии, но в т.ч. красиво перекладывается на код и доменные модели.
There was a problem hiding this comment.
sourceText сюда передается с мыслью, чтобы токен мог достать свое значение из текста, в котором он существует. Соглашусь, что может выглядеть как переизбыток данных, но пока я еще не дописал полностью логику генератора хтмл, пусть он полежит тут (хотя бы для дебаг целей)
| public int Length { get; } = length; | ||
| public TokenType TokenType { get; } = tokenType; | ||
|
|
||
| public string GetValue() |
There was a problem hiding this comment.
Мб свойством? Тут логики не особо много)
|
|
||
| public override string ToString() | ||
| { | ||
| return $"Token {TokenType} | Value \"{GetValue()}\""; |
There was a problem hiding this comment.
NIT: Лучше не переопределять .ToString без лишней потребности. Не совсем очевидное поведение. В плане, без углубления в код нельзя понять, что .ToString делает именно это (я бы первым делом подумал, что он возвращает value, это стандартное поведение таких методов)
p.s. а если хочешь оставить что-то подобное для дебага, то можно заюзать Externsion метод по типу .ToDebugString
There was a problem hiding this comment.
Да да, знаю, что переопределять ту стринг это харам. Но это осмысленный харам, так как я хотел, чтобы мне дебагер писал, что лежит в листе, когда я по шагам гуляю. А если делать как предложил ты, то среда не подтянет этот метод, когда я буду в дебаге смотреть код
| UNDERSCORE, | ||
| SPACE, | ||
| NEW_LINE, | ||
| NUMBER, |
There was a problem hiding this comment.
А чем NUMBER отличается от TEXT с точки зрения обработки?
There was a problem hiding this comment.
И похожий вопрос на засыпку: для чего хочешь использовать SPACE?)
There was a problem hiding this comment.
NUMBER и TEXT отличаются тем, что число с _ воспринимается как число, а текст - как тег. SPACE же нужен, чтобы хендлить вариант, когда у меня будет разрыв тегов между слов wor_d hel_lo. Если не определить пробел, то сканер вернет мне последовательность TEXT _ TEXT _ TEXT которую легко спутать с тегом
| { | ||
| public static void Main(string[] args) | ||
| { | ||
| var markdown = "This _is_ a __sample__ markdown _file_.\n"; |
There was a problem hiding this comment.
Советы категории Б: можешь вынести эту строчку в константу, чтобы потом, при добавлении новых примеров, можно было легко между ними переключаться :)
const string firstExample = "This _is_ a __sample__ markdown _file_.\n";
const string secondExample = "#This is another __sample__ markdown _file_";
var markdown = firstExample;
| var token = scanners | ||
| .Select(sc => sc.Scan(markdown, begin)) | ||
| .First(token => token is not null); | ||
| begin += token!.Length; tokenList.Add(token); |
There was a problem hiding this comment.
Перенеси на новую строчку плиз, глазкам больно ахаха
| if (tokenType is null) return null; | ||
|
|
||
| var notNullType = (TokenType)tokenType; | ||
| return new Token(notNullType, begin, 1, markdown); |
There was a problem hiding this comment.
А как будешь разбираться с bold? Это, если что, __. Тут уже два символа. К GetTokenType тот же вопрос)
There was a problem hiding this comment.
Как я описывал выше: токены это язык, который мой автомат собирается распознавать. Поэтому болд для меня это +- такая вещь __ TEXT __
There was a problem hiding this comment.
Не. я к тому, что сейчас все заточено под 1 символ (абсолютно все). bold в 2 символа может добавить проблем, если сразу не подумать над ним.
Если в следующих коммитах уже есть решение, то гуд)
| } | ||
|
|
||
| public static bool CanScan(char symbol) | ||
| => GetTokenType(symbol) != null; |
There was a problem hiding this comment.
А если у нас будет текст вида Всем_привет!?
Потом будешь разбирать в автомате? Кажется, все сломается. Ну, у меня ощущение такое.
There was a problem hiding this comment.
Оно не должно сломаться, так как автомат не найдет закрывающей _ и просто проскочит на следующий символ
|
|
||
| private static TokenType? GetTokenType(char symbol) => symbol switch | ||
| { | ||
| ' ' => TokenType.SPACE, |
There was a problem hiding this comment.
NIT: мб словарем сделать? Удобнее будет :)
https://marcduerst.com/2019/10/30/mapping-with-dictionary-instead-switch/
There was a problem hiding this comment.
Словарь так словарь: ща поправлю :)
There was a problem hiding this comment.
Не, NIT - это буквально "докопаться". Такие треды можешь не исправлять, если не хочешь/считаешь свое решение лучше. Я тут просто варианты накидываю)
- Немного пошаманил с неймингом - Чутка переделал ноду
- Отказался от метч объектов - Поделил ноды на текст ноду и тег ноду - Добавил разных экстеншенов, чтобы код был более читабельным
- Теперь италик тег умеет определять, когда он метчит тег в слове или вне его
- Тесты на пробел перед закрывающим - Тесты на пробел после открывающего
cs/Markdown/Parser/Nodes/Node.cs
Outdated
| @@ -0,0 +1,7 @@ | |||
| namespace Markdown.Parser.Nodes; | |||
|
|
|||
| public class Node(NodeType nodeType, int consumed) | |||
There was a problem hiding this comment.
Не понимаю, за что отвечает int Consumed. Это не Length случайно?
cs/Markdown/Parser/Nodes/TextNode.cs
Outdated
|
|
||
| namespace Markdown.Parser.Nodes; | ||
|
|
||
| public class TextNode(int start, int consumed, List<Token> source) : Node(NodeType.TEXT, consumed) |
There was a problem hiding this comment.
А вот тут точно нарушение SRP и всего вместе взятого (или я не понимаю). Почему сюда весь source закидывается?.. Почему не только нужные токены?
cs/Markdown/Parser/Rules/BoldRule.cs
Outdated
| { | ||
| public Node? Match(List<Token> tokens, int begin = 0) | ||
| { | ||
| throw new NotImplementedException(); |
| } | ||
|
|
||
| private static TagNode BuildNode(TextNode textNode) | ||
| => new(NodeType.ITALIC, textNode, textNode.Consumed + 2); |
There was a problem hiding this comment.
Я бы не завязывался на + 2. В md есть разные форматы, поэтому лучше вынести в константу
There was a problem hiding this comment.
И как будто есть смысл вынести такой Factory метод в сами ноды, как думаешь?
| public Node? Match(List<Token> tokens, int begin = 0) | ||
| { | ||
| if (pattern.Count == 0) return null; | ||
| if (tokens.Count - begin < pattern.Count) return null; |
There was a problem hiding this comment.
NIT: почему думаешь, что лучше вернуть null, чем выкинуть Exception?
| { | ||
| var node = pattern.Match(tokens, begin); | ||
| if (node is null) return nodes; | ||
| begin += node.Consumed; nodes.Add(node); |
| public static class ListExtensions | ||
| { | ||
| public static string ToText(this List<Token> tokens) | ||
| => tokens.Aggregate(new StringBuilder(), (sb, t) => sb.Append(t.Value)).ToString(); |
cs/Markdown/Parser/Rules/TextRule.cs
Outdated
| } | ||
|
|
||
| private static bool IsText(Token token) | ||
| => token.TokenType is TokenType.WORD or TokenType.SPACE; |
- Италик тег стал еще более устойчив - Код похорошел, новые плюхи появились, плохие ушли
- Более семантично теперь
- Более стойкий болд тег - Более строгие тесты
- Добавил новый токен - Добавил новый тип нод - Поправил тестики
- Добавил тег параграфа - Накидал тестов для тега - Поправил тег инворд италик тег - Поправил инворд болд тег
- Начал рефакторинг тегов
- Вернулся к кондитионал руле - Честно прописал условия для границ тегов подчерка
- Некоторые тесты перекочевали в InWord аналоги
- Решил переименовать булевы рулы в специальный рулы (чем они и являются)
- Теперь все еквивалент матчи строги к порядку
- Теперь ноды не хранят в себе лист токенов - Пошел править все ноды с этим обновлением
- Переделал италик тег на новые ноды - Поменял токены для уменьшения их ответственности
- Добавил тесты для заголовка - Переименовал метчи в ноды в некоторых тестках
- Добавил новый тип токена - Поправил тесты и сканеры - Поправил рулы параграфа - Написал сам тег + тесты
- Ескейп тег теперь настоящий тег - Ескейп тег принимает токены, которые он будет ескейпить - Параграф теперь умеет ескейпить все андерскоры
@w1jtoo