-
Notifications
You must be signed in to change notification settings - Fork 319
Шагов Владислав #268
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?
Шагов Владислав #268
Conversation
cs/Markdown/Markdown.cs
Outdated
| @@ -0,0 +1,16 @@ | |||
| namespace Markdown; | |||
|
|
|||
| public static class Markdown | |||
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.
Про именование - "В fork-е этого репозитория создай проект Markdown и реализуй метод Render класса Md." То, что ты решил назвать по-своему не грубая ошибка, но усложняет проверку.
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/Md.cs
Outdated
| { | ||
| public static string ToHtml(string markdownText) | ||
| { | ||
| var tokenizer = new Tokenizer(markdownText); |
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.
tokenizer, text-parser и renderer предлагаю вынести в конструктор - либо принимай их там в качестве аргументов и один раз сохраняй в полях класса, либо создавай прямо в конструкторе; иначе получается, что на каждый вызов метода ты заново создаёшь экземпляры классов и повышаешь трафик памяти; дополнительно в токенайзере написал про это
cs/Markdown/MarkdownTests.cs
Outdated
| @@ -0,0 +1,24 @@ | |||
| using NUnit.Framework; | |||
| using System; | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/MarkdownTests.cs
Outdated
| using NUnit.Framework; | ||
| using System; | ||
|
|
||
| namespace Markdown; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/MarkdownTests.cs
Outdated
|
|
||
| namespace Markdown; | ||
| [TestFixture] | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Tokenizer.cs
Outdated
| private char Current => _position < _text.Length ? _text[_position] : '\0'; | ||
| private char Peek(int offset = 1) | ||
| { | ||
| int pos = _position + offset; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Tokenizer.cs
Outdated
| return tokens; | ||
| } | ||
|
|
||
| private bool TryReadEscape(List<Token> tokens) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Tokenizer.cs
Outdated
| return true; | ||
| } | ||
|
|
||
| private bool TryReadFormatting(List<Token> tokens) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Tokenizer.cs
Outdated
| if (Current == '_' && Peek() == '_') | ||
| { | ||
| Advance(2); | ||
| tokens.Add(new Token(TokenType.BoldStart, "__")); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/HtmlRenderer.cs
Outdated
| { | ||
| public string Render(List<Node> nodes) | ||
| { | ||
| var sb = new StringBuilder(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/MarkdownTests.cs
Outdated
| public string HeadingAndParagraphs(string content) | ||
| { | ||
|
|
||
| content = Markdown.ToHtml(content); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| [Test] | ||
| public void ConvertToHtml_ShouldParseMultipleLists_WhenSeparatedByContent() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| content.Should().Be("<ul><li>List A1</li><li>List A2</li></ul>\n<p>Paragraph</p>\n<ul><li>List B1</li><li>List B2</li></ul>"); | ||
| } | ||
| [Test] | ||
| public void ConvertToHtml_ShouldParseHeading_WhenStartWithOctothorpe() |
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.
Этот тест-кейс есть в следующем тесте.
| [TestCase("#Heading", "<h1>Heading</h1>")] | ||
| [TestCase("##Heading", "<h2>Heading</h2>")] | ||
| [TestCase("###Heading", "<h3>Heading</h3>")] | ||
| [TestCase("####Heading", "<h4>Heading</h4>")] |
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.
- Некорректно работает с кейсом "#################################"
- Некорректно работает с кейсом "\##Heading"
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.
1 не соглашусь, если просто "##############" то "", если "##############Heading" то "<h6>Heading</h6>"
установлено ограничение на 6 уровней заголовка, если исправлять, то мне кажется лучше в документации указать просто, что в HTML только 6 уровней заголовков.
2 в данном случае должно вывестись <p>#</p>\n<h1>Heading</h1>? или же <p>##Heading</p>? в спецификации не указан такой случай из-за отсутствия многоуровневых заголовков, смею предположить что первый вариант выглядит логичнее, но при этом ломает представление, тк заголовок и параграф не должны быть в одной строке.
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.
Обсудили в телеге
| [Test] | ||
| public void ConvertToHtml_ShouldNorParse_WhenOnlyUnderscores() | ||
| { | ||
| var 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.
- Здесь и в других тестах ненужная "@".
- В этом тесте много кейсов, подумай, как следует.
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.
- Некорректно работает с немного другим кейсом
"__ __"(именно двойные подчёркивания).
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.
3 вижу странный результат, как в таком случае должны обрабатываться эти подчёркивания? обрабатываться просто как подчёркивания?
в спецификации два кейса:
1 За подчерками, начинающими выделение, должен следовать непробельный символ. Иначе эти_ подчерки_ не считаются выделением
и остаются просто символами подчерка.
2 Если внутри подчерков пустая строка ____, то они остаются символами подчерка.
но строка вроде и не пустая, но при этом с пробелами частично, что и выдаёт результат "_ __"
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.
Обсудили в телеге
| } | ||
|
|
||
| [Test] | ||
| public void ConvertToHtml_ShouldParse_WhenScreeningOctothorpe() |
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.
Объедини с другими кейсами про заголовки или с другими кейсами про экранирование.
|
|
||
| content.Should().Be("<p><strong>bold text</strong></p>"); | ||
| } | ||
|
|
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.
Чет много места между тестами
|
|
||
|
|
||
|
|
||
| [Test] |
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.
Некорректно работает с кейсом _Italic Heading\\\\_ - я здесь экранирую слэш, значит, должно вернуться Italic Heading\, так?
|
|
||
| content.Should().Be("<p>#Start</p>"); | ||
| } | ||
|
|
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.
- Некорректно работает кейс
"\\"- символ экранирования должен исчезать, только если экранирует что-то, не правда ли? - Подумай о случаях, когда вложенность экранирования больше 1-2 символов.
| } | ||
|
|
||
| [Test] | ||
| public void ConvertToHtml_ShouldParse_WhenScreeningSlash() |
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.
Отсутствует перфоманс-тест.
| content.Should().Be("<p>Start _something _now</p>"); | ||
| } | ||
| [Test] | ||
| public void ConvertToHtml_ShouldParse_WhenScreeningUnderscore() |
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.
Работает с кейсом "_he_ll_", но почему-то не работает с кейсом "_._._".
| public TokenType Type { get; } | ||
| public string Value { get; } | ||
|
|
||
| public Token(TokenType type, string value = "") |
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.
Да, только конструктор приватный надо было сделать.
@Dimques