Мажирин Александр #237

Kexogg · 2024-11-24T23:14:04Z

No description provided.

GlazProject · 2024-11-26T14:48:31Z

cs/Markdown/Markdown/Md.cs

+{
+    public string Render(string md)
+    {
+        var tokenizer = new Tokenizer();


Выглядит как что-то, что можно вынести в приватное поле и генерировать только при создании класса Md

GlazProject · 2024-11-26T14:49:15Z

cs/Markdown/Markdown/Md.cs

+    {
+        var tokenizer = new Tokenizer();
+        var tokens = tokenizer.Tokenize(md);
+        var renderer = new HtmlRenderer();


аналогично предыдущему. Не требует специфичных данных метода и не похоже, чтобы сохранял во внутреннюю структуру что-то

GlazProject · 2024-11-26T14:56:36Z

cs/Markdown/Markdown.csproj

+
+    <PropertyGroup>
+        <OutputType>Exe</OutputType>
+        <TargetFramework>net9.0</TargetFramework>


net9 только вышел из беты, поэтому активно пока не применяется. Лучше по началу использовать стабильные версии с долгосрочной поддержкой. Такие как 8, например

GlazProject · 2024-11-26T14:58:25Z

cs/Markdown/Program.cs

+    static void Main(string[] args)
+    {
+        var mdFile = File.ReadAllText("Markdown.md");
+        var md = new Markdown.Md();


Точно нужно указывать полное имя класса с namespace?

GlazProject · 2024-11-26T14:59:51Z

cs/Markdown/Program.cs

+    {
+        var mdFile = File.ReadAllText("Markdown.md");
+        var md = new Markdown.Md();
+        Console.WriteLine(md.Render(mdFile));


Для тестов вполне достаточно выводить в консоль. Но если делать какой-то cli, то лучше сохранять в файлик html рядом с входным

GlazProject · 2024-11-26T15:39:42Z

cs/Markdown/Token/IToken.cs

@@ -0,0 +1,6 @@
+namespace Markdown.Token;
+
+public interface IToken


токен больше похож на data class, нежели что-то с различными реализациями. Расскажи, что вкладываешь в него и как планируешь использовать при парсинге?

GlazProject · 2024-11-26T15:41:15Z

cs/MarkdownTests/MarkdownTests.cs

+    [Test]
+    public void Render_ShouldReturnSimpleText()
+    {
+        md.Render("Hello, world!").Should().Be("Hello, world!");


Вынесем текст в константу метода? Чтобы точно не менялась )

GlazProject · 2024-11-26T15:41:57Z

cs/MarkdownTests/MarkdownTests.cs

+
+namespace MarkdownTests;
+
+public class MarkdownTests


Класс всё-таки Md, хоть под ним и подразумевается Markdown

GlazProject · 2024-11-26T15:45:17Z

cs/MarkdownTests/MarkdownTests.cs

+    [Test]
+    public void Render_ShouldReturnImage()
+    {
+        md.Render("(Hello, image!)[img]").Should().Be("<img alt=\"Hello, image!\" src=\"img\"/>");


Этот тег получается ты придумал сам? В md для картинок применяется ![alt text](http://url/to/img.png)

GlazProject · 2024-11-26T15:48:34Z

cs/MarkdownTests/MarkdownTests.cs

+
+namespace MarkdownTests;
+
+public class MarkdownTests


Весь этот класс в целом тестирует Md как чёрный ящик, что довольно сложно станет при разработке. Есть предложение сократить здесь количество тестов, а сделать отдельные классы тестов для токенайзера и генератора. Таким образом сразу будет понятно, где ошибка: при парсинге или при рендеринге

GlazProject · 2024-12-01T11:13:53Z

cs/MarkdownTests/MarkdownTests.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+    <PropertyGroup>
+        <TargetFramework>net9.0</TargetFramework>


А в тестах всё ещё net9 прячется

GlazProject · 2024-12-01T11:15:42Z

cs/MarkdownTests/MarkdownTokenizerTests.cs

+    [TestFixture]
+    public class MarkdownTokenizerTests
+    {
+        private MarkdownTokenizer _tokenizer;


Не стоит смешивать правила нейминга в рамках одного проекта. Ранее для приватных полей использовал уже просто с маленькой буквы. Нет единого правильного варианта - в каждом проекте решают по своему, как писать. Главное одинаково

GlazProject · 2024-12-01T11:17:02Z

cs/MarkdownTests/HtmlRendererTests.cs

+        }
+
+        [Test]
+        public void Render_ShouldRenderTagTokenWithAttributes()


Больше похоже на рендер картинки

GlazProject · 2024-12-01T11:21:14Z

cs/MarkdownTests/MarkdownTokenizerTests.cs

+                }
+            }
+
+            _tokenizer = null;


В этом нет особого смысла, так как с ним больше ничего нигде не происходит, а в SetUp ты его назначаешь. В основном в TearDown производят сохранение ошибок, восстановление общих настроек. В OneTimeTearDown могут выполнять dispose каких-то переменных.

GlazProject · 2024-12-01T11:31:51Z

cs/MarkdownTests/MarkdownTokenizerTests.cs

+            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>();


Получится сделать с использованием .Which?

GlazProject · 2024-12-07T11:23:38Z

cs/Markdown/Tokenizer/MarkdownTokenTreeBuilder.cs

+            return tree;
+        }
+
+        foreach (var tag in tagTokens)


Очень длинный и нечитаемый код. Нужно либо разбивать на методы с понятными названиями, либо переосмысливать происходящее

GlazProject · 2024-12-07T11:29:08Z

cs/Markdown/Tokenizer/MarkdownTokenizer.cs

+{
+    private readonly MarkdownRules markdownRules = new();
+
+    private readonly List<ITag> tags = new()


Скорее supportedTags

GlazProject · 2024-12-07T11:32:19Z

cs/Markdown/Tokenizer/MarkdownTokenizer.cs

+        for (var i = 0; i < content.Length; i++)
+            foreach (var tag in tags)
+            {
+                var tagToken = new TagToken(tag) { Position = i };


Он создаёт новый токен на КАЖДЫЙ символ на КАЖДЫЙ поддерживаемый тэг?!

GlazProject · 2024-12-07T11:48:06Z

cs/Markdown/Tokenizer/Rules/MarkdownRules.cs

+        {
+            IsTag = (tagToken, content) =>
+                (content.ContainsSubstringOnIndex(tagToken.Tag.MdClosingTag, tagToken.Position)
+                 && content[..tagToken.Position].LastIndexOf('#') > content[..tagToken.Position].LastIndexOf('\n')) ||


Для каждого вызова создаём новую строку content[..tagToken.Position] (дважды), за ещё одну линейную сложность .LastIndexOf('#') (дважды) не находим нужный символ. Потратили кучу памяти и много времени!

GlazProject · 2024-12-07T12:12:25Z

cs/MarkdownTests/MdTests.cs

+
+namespace MarkdownTests;
+
+public class MdTests


Вот такой тест к сожалению не пройдёт. Теряются переносы строк (учитываю, что они заменяются в твоей реализации на
, но так лучше не делать) , дублируется текст в strong и не совсем корректно обрабатывается курсив в жирном и пересечение выделений.

private const string Text = "#заголовок __с жирным текстом__\n" + "Просто текст, в котором _курсивные_ выделения\n" + "__Есть жирный текст__\n" + "__А вот жирный текст _с курсивом_ внутри _и ещё курсив_ в жирном__\n" + "_Вот __это_ не__ сработает\n" + "_И вот так __тоже__ нет_\n" + "Это - _ - просто подчёркивание\n" + "Так_ не работает_\n" + "И _ вот так _ тоже\n" + "В с_лов_е можно выделять, а в цифрах 1_23_ нет\n" + "Ещу можно сделать просто _курсив_\n"; private const string ExpectedResult = "<h1>заголовок с жирным текстом</h1> " + "Просто текст, в котором курсивные выделения " + "Есть жирный текст " + "А вот жирный текст с курсивом внутри и ещё курсив в жирном " + "_Вот __это_ не__ сработает " + "_И вот так __тоже__ нет_ " + "Это - _ - просто подчёркивание " + "Так_ не работает_ " + "И _ вот так _ тоже " + "В слове можно выделять, а в цифрах 1_23_ нет " + "Ещу можно сделать просто курсив "; [Test] public void Render_ShouldReturn_HtmlString() { var result = new Md().Render(Text); result.Should().Be(ExpectedResult); }

GlazProject · 2024-12-15T16:39:12Z

cs/Markdown/Tokenizer/Tokens/TagToken.cs

+/// </summary>
+/// <param name="content">Содержание</param>
+/// <param name="tag">Название тега</param>
+public class TagToken(ITag tag) : IToken


kontur-courses/tdd#252 (comment)

GlazProject · 2024-12-15T16:39:25Z

cs/Markdown/Tokenizer/Tokens/TextToken.cs

+/// Текстовый токен. Содержит чистый текст
+/// </summary>
+/// <param name="content">Текст</param>
+public class TextToken(string content) : IToken


kontur-courses/tdd#252 (comment)

GlazProject · 2024-12-15T16:56:09Z

cs/MarkdownTests/MarkdownTokenizerPerformanceTests.cs

+
+    private void TokenizePerformanceTest(string filePath, int maxMilliseconds)
+    {
+        var content = File.ReadAllText(filePath);


Это ведь шаблонный файл. Не хочешь его генерировать на лету в память, а не сохранять отдельно?

GlazProject · 2024-12-15T16:58:15Z

cs/MarkdownTests/MarkdownTokenizerPerformanceTests.cs

+        var memoryBefore = GC.GetTotalMemory(true);
+        var result = tokenizer.Tokenize(content);
+        var memoryAfter = GC.GetTotalMemory(false);


Не очень честный метод измерения, так как за время теста GC может всё почистить и размер получится вообще отрицательным. Но хорошо, что знаешь про API для обращения к нему

GlazProject · 2024-12-15T17:04:22Z

cs/Markdown/Markdown/Md.cs

 public class Md : IMd
 {
+    private readonly ITokenizer tokenizer = new MarkdownTokenizer();


А теперь токенайзер не хранит никакое состояние и может стать полноценным приватным полем )

GlazProject · 2024-12-15T17:41:27Z

cs/MarkdownTests/MarkdownTokenizerPerformanceTests.cs

+    [Test]
+    public void Tokenize_HeaderPerformanceTest()
+    {
+        TokenizePerformanceTest("TestCases/MarkdownTokenizer/" + TestContext.CurrentContext.Test.MethodName + ".md", 200);


У меня этот тест не проходит периодически, занимает 230ms. Думаю, можно ему увеличить порог немного

Kexogg added 2 commits November 25, 2024 04:11

initial markdown commit

913f25a

Tests

972bcab

GlazProject reviewed Nov 26, 2024

View reviewed changes

Kexogg added 20 commits November 27, 2024 21:06

address first review issues

8eabce6

add attributes to tag token

753c342

initial markdown, renderer & tests

305ed01

update tests

5990650

check for disallowed tags

07b89a5

implement markdown rules

c419829

refactor word scanner

4db979e

check for newline when parsing headers

0082678

fix unpaired tags

e060481

add check for intersecting tags

137ccca

image tag support

98ca9b2

escape

6d1085b

cleanup

a047313

correctly handle newline tokens

e1eeee1

fix header rules

ee313cd

refactor

3868044

remove old tests

ae342db

update tests, escape tag fixes

37fd3d6

minor refactoring

950b252

fix todo's

1cbfbb7

GlazProject suggested changes Dec 7, 2024

View reviewed changes

Kexogg added 6 commits December 13, 2024 05:47

minor refactoring

5ad3700

performance fixes

3c126af

refactor treebuilder

ff94e0c

fix handling of newline after header tag

9e7a71b

fix double text

286d18c

fix overlaps

4a5ea48

Kexogg added 2 commits December 14, 2024 05:14

tests

a3b7e56

remove GetTokens()

492d7e4

GlazProject reviewed Dec 15, 2024

View reviewed changes

final fixes

6d360a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Мажирин Александр #237

Мажирин Александр #237

Kexogg commented Nov 24, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Nov 26, 2024

GlazProject Dec 1, 2024

GlazProject Dec 1, 2024

GlazProject Dec 1, 2024

GlazProject Dec 1, 2024

GlazProject Dec 1, 2024

GlazProject Dec 7, 2024

GlazProject Dec 7, 2024

GlazProject Dec 7, 2024

GlazProject Dec 7, 2024

GlazProject Dec 7, 2024

GlazProject Dec 15, 2024

GlazProject Dec 15, 2024

GlazProject Dec 15, 2024

GlazProject Dec 15, 2024

GlazProject Dec 15, 2024

GlazProject Dec 15, 2024

		@@ -0,0 +1,6 @@
		namespace Markdown.Token;

		public interface IToken

Мажирин Александр #237

Are you sure you want to change the base?

Мажирин Александр #237

Conversation

Kexogg commented Nov 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment