-
Notifications
You must be signed in to change notification settings - Fork 319
Кроткая Александра #264
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?
Кроткая Александра #264
Conversation
Zuguki
left a comment
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 Markdown.Core.Rendering; | ||
|
|
||
| public interface IHtmlRenderer |
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.
Почему тут именно HtmlRenderer? Кажется что можно сделать интефейс более абстрактным, например, IRender, а уже реализация будет HtmlRender
Реализован рендеринг ссылки, все тесты проходятся, код почистила
Zuguki
left a comment
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.Core/Lexing/Lexer.cs
Outdated
|
|
||
| switch (symbol) | ||
| { | ||
| case '#' when IsAtLineStart(tokens) && i + 1 < length && tokensSpan[i + 1] == ' ': |
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.
\t еще стоит обрабатывать
cs/Markdown.Core/Lexing/Token.cs
Outdated
| /// <summary> | ||
| /// Одна часть (токен) входного текста | ||
| /// </summary> | ||
| public readonly struct Token(TokenKind kind, ReadOnlyMemory<char> slice, int position) |
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.
position не используется в итоге, можно убрать
cs/Markdown.Core/Lexing/TokenKind.cs
Outdated
| Space, //Одиночный пробел ' ', нужен для проверки границ выделений | ||
| NewLine, //Перевод строки (например, '\n' или '\r\n') | ||
| Eof // Служебный маркер конца входа | ||
| Text, // Обычный текст |
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.
лучше писать это в summary, так как при наведении на тип будет показываться описание
cs/Markdown.Core/Lexing/Lexer.cs
Outdated
| i += 1; | ||
| continue; | ||
|
|
||
| case '#': |
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.
если четсно - немного костыльно выглядит) лучше уж в case сделать if else, чем два case на 1 символ делать
| /// - выделяет специальные токены | ||
| /// - применяет базовые правила экранирования | ||
| /// </summary> | ||
| public class Lexer : ILexer |
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.
вообще класс содержит в себе много ответсвенности. Считай он бежит по строке и разбивает ее на токены, при этом содержит еще и логику разбиения на токены.
Предлагаю чуть поднять тут srp и сделать более расширяемым, при этом не сильно меняя. Для этого можно сделать Handler/Processon на каждый символ, тут будем так же в switchе проверять символ, в зависимости от которого будем вызывать соответсвующий Handler, который уже сделает обработку и вернет токен
| /// - внутри блоков собирает инлайны (Текст/Курсив/Жирный) | ||
| /// - следует спецификации (границы, цифры, пересечения, пустые выделения) | ||
| /// </summary> | ||
| public class Parser : IParser |
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.
тут тоже намудрила) тут тоже класс слишком много на себе берет, давай его поделим. Например, у тебя тут отдельно парсятся параграфы и возвращают свои ноды, так же и другие. Тут бы логику попилить, отдельно сделать парсеры для токенов, тут класс идейно должнен просто в нужном порядке вызывать соответсвующий парсер, а сам парсер вернет ноду
@Zaguki