-
Notifications
You must be signed in to change notification settings - Fork 319
Медников Матвей #253
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?
Медников Матвей #253
Conversation
cs/Markdown/HtmlRender/HtmlRender.cs
Outdated
| RenderNodes(doc.Children, sb); | ||
| break; | ||
| } | ||
| } |
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/HtmlRender/HtmlRender.cs
Outdated
| RenderNodes(heading.Children, sb); | ||
|
|
||
| sb.Append("</h1"); | ||
| sb.Append(">"); |
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/HtmlRender/HtmlRender.cs
Outdated
|
|
||
| private static void RenderNode(Node node, StringBuilder sb) | ||
| { | ||
| switch (node) |
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/Lexer/Lexer.cs
Outdated
| { | ||
| var c = cursor.Current; | ||
|
|
||
| switch (c) |
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.
Тут то же самое. Понятно, есть уникальная логика для двойной земли, но остальное можно объединить
Да я бы и земли тоже попытался объединить, тк, например, кроме заголовка h1 есть h2, h3, и они обозначаются двумя и тремя решётками. То есть вообще нет гарантий, что не надо будет добавлять в обработчик тег, обрабатываемый одним символом
cs/Markdown/Parser/Parser.cs
Outdated
| indexer = new TokenIndexer(tokens); | ||
| } | ||
|
|
||
| public static DocumentNode Parse(List<Token> tokens) |
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.
Можно написать
public DocumentNode Parse() // без static
{
var nodes = ParseDocument();
return new DocumentNode(nodes);
}
и вызывать это так:
new Parser(tokens).Parse()
Шо то шо это, просто статический метод ломает полиморфизм через колено
No description provided.