Мажирин Александр #237
Мажирин Александр #237
Conversation
cs/Markdown/Markdown/Md.cs
Outdated
| { | ||
| public string Render(string md) | ||
| { | ||
| var tokenizer = new Tokenizer(); |
There was a problem hiding this comment.
Выглядит как что-то, что можно вынести в приватное поле и генерировать только при создании класса Md
cs/Markdown/Markdown/Md.cs
Outdated
| { | ||
| var tokenizer = new Tokenizer(); | ||
| var tokens = tokenizer.Tokenize(md); | ||
| var renderer = new HtmlRenderer(); |
There was a problem hiding this comment.
аналогично предыдущему. Не требует специфичных данных метода и не похоже, чтобы сохранял во внутреннюю структуру что-то
cs/Markdown/Markdown.csproj
Outdated
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net9.0</TargetFramework> |
There was a problem hiding this comment.
net9 только вышел из беты, поэтому активно пока не применяется. Лучше по началу использовать стабильные версии с долгосрочной поддержкой. Такие как 8, например
cs/Markdown/Program.cs
Outdated
| static void Main(string[] args) | ||
| { | ||
| var mdFile = File.ReadAllText("Markdown.md"); | ||
| var md = new Markdown.Md(); |
There was a problem hiding this comment.
Точно нужно указывать полное имя класса с namespace?
cs/Markdown/Program.cs
Outdated
| { | ||
| var mdFile = File.ReadAllText("Markdown.md"); | ||
| var md = new Markdown.Md(); | ||
| Console.WriteLine(md.Render(mdFile)); |
There was a problem hiding this comment.
Для тестов вполне достаточно выводить в консоль. Но если делать какой-то cli, то лучше сохранять в файлик html рядом с входным
cs/Markdown/Token/IToken.cs
Outdated
| @@ -0,0 +1,6 @@ | |||
| namespace Markdown.Token; | |||
|
|
|||
| public interface IToken | |||
There was a problem hiding this comment.
токен больше похож на data class, нежели что-то с различными реализациями. Расскажи, что вкладываешь в него и как планируешь использовать при парсинге?
cs/MarkdownTests/MarkdownTests.cs
Outdated
| [Test] | ||
| public void Render_ShouldReturnSimpleText() | ||
| { | ||
| md.Render("Hello, world!").Should().Be("Hello, world!"); |
There was a problem hiding this comment.
Вынесем текст в константу метода? Чтобы точно не менялась )
cs/MarkdownTests/MarkdownTests.cs
Outdated
|
|
||
| namespace MarkdownTests; | ||
|
|
||
| public class MarkdownTests |
There was a problem hiding this comment.
Класс всё-таки Md, хоть под ним и подразумевается Markdown
cs/MarkdownTests/MarkdownTests.cs
Outdated
| [Test] | ||
| public void Render_ShouldReturnImage() | ||
| { | ||
| md.Render("(Hello, image!)[img]").Should().Be("<img alt=\"Hello, image!\" src=\"img\"/>"); |
There was a problem hiding this comment.
Этот тег получается ты придумал сам? В md для картинок применяется 
cs/MarkdownTests/MarkdownTests.cs
Outdated
|
|
||
| namespace MarkdownTests; | ||
|
|
||
| public class MarkdownTests |
There was a problem hiding this comment.
Весь этот класс в целом тестирует Md как чёрный ящик, что довольно сложно станет при разработке. Есть предложение сократить здесь количество тестов, а сделать отдельные классы тестов для токенайзера и генератора. Таким образом сразу будет понятно, где ошибка: при парсинге или при рендеринге
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net9.0</TargetFramework> |
| [TestFixture] | ||
| public class MarkdownTokenizerTests | ||
| { | ||
| private MarkdownTokenizer _tokenizer; |
There was a problem hiding this comment.
Не стоит смешивать правила нейминга в рамках одного проекта. Ранее для приватных полей использовал уже просто с маленькой буквы. Нет единого правильного варианта - в каждом проекте решают по своему, как писать. Главное одинаково
| } | ||
|
|
||
| [Test] | ||
| public void Render_ShouldRenderTagTokenWithAttributes() |
| } | ||
| } | ||
|
|
||
| _tokenizer = null; |
There was a problem hiding this comment.
В этом нет особого смысла, так как с ним больше ничего нигде не происходит, а в SetUp ты его назначаешь. В основном в TearDown производят сохранение ошибок, восстановление общих настроек. В OneTimeTearDown могут выполнять dispose каких-то переменных.
| tokens.Should().HaveCount(1); | ||
| tokens[0].Should().BeOfType<TagToken>(); | ||
| tokens[0].Children![0].TextContent.Should().Be("strong text"); | ||
| ((TagToken)tokens[0]).Tag.Should().BeOfType<StrongTag>(); |
There was a problem hiding this comment.
Получится сделать с использованием .Which?
| return tree; | ||
| } | ||
|
|
||
| foreach (var tag in tagTokens) |
There was a problem hiding this comment.
Очень длинный и нечитаемый код. Нужно либо разбивать на методы с понятными названиями, либо переосмысливать происходящее
| { | ||
| private readonly MarkdownRules markdownRules = new(); | ||
|
|
||
| private readonly List<ITag> tags = new() |
| for (var i = 0; i < content.Length; i++) | ||
| foreach (var tag in tags) | ||
| { | ||
| var tagToken = new TagToken(tag) { Position = i }; |
There was a problem hiding this comment.
Он создаёт новый токен на КАЖДЫЙ символ на КАЖДЫЙ поддерживаемый тэг?!
| { | ||
| IsTag = (tagToken, content) => | ||
| (content.ContainsSubstringOnIndex(tagToken.Tag.MdClosingTag, tagToken.Position) | ||
| && content[..tagToken.Position].LastIndexOf('#') > content[..tagToken.Position].LastIndexOf('\n')) || |
There was a problem hiding this comment.
Для каждого вызова создаём новую строку content[..tagToken.Position] (дважды), за ещё одну линейную сложность .LastIndexOf('#') (дважды) не находим нужный символ. Потратили кучу памяти и много времени!
|
|
||
| namespace MarkdownTests; | ||
|
|
||
| public class MdTests |
There was a problem hiding this comment.
Вот такой тест к сожалению не пройдёт. Теряются переносы строк (учитываю, что они заменяются в твоей реализации на
, но так лучше не делать) , дублируется текст в strong и не совсем корректно обрабатывается курсив в жирном и пересечение выделений.
private const string Text = "#заголовок __с жирным текстом__\n" +
"Просто текст, в котором _курсивные_ выделения\n" +
"__Есть жирный текст__\n" +
"__А вот жирный текст _с курсивом_ внутри _и ещё курсив_ в жирном__\n" +
"_Вот __это_ не__ сработает\n" +
"_И вот так __тоже__ нет_\n" +
"Это - _ - просто подчёркивание\n" +
"Так_ не работает_\n" +
"И _ вот так _ тоже\n" +
"В с_лов_е можно выделять, а в цифрах 1_23_ нет\n" +
"Ещу можно сделать просто _курсив_\n";
private const string ExpectedResult = "<h1>заголовок <strong>с жирным текстом</strong></h1><br />" +
"Просто текст, в котором <em>курсивные</em> выделения<br />" +
"<strong>Есть жирный текст</strong><br />" +
"<strong>А вот жирный текст <em>с курсивом</em> внутри <em>и ещё курсив</em> в жирном</strong><br />" +
"_Вот __это_ не__ сработает<br />" +
"_И вот так __тоже__ нет_<br />" +
"Это - _ - просто подчёркивание<br />" +
"Так_ не работает_<br />" +
"И _ вот так _ тоже<br />" +
"В с<em>лов</em>е можно выделять, а в цифрах 1_23_ нет<br />" +
"Ещу можно сделать просто <em>курсив</em><br />";
[Test]
public void Render_ShouldReturn_HtmlString()
{
var result = new Md().Render(Text);
result.Should().Be(ExpectedResult);
}
No description provided.