Сазонов Александр #233

AlexxSaz · 2024-11-24T15:08:49Z

dmitgaranin · 2024-11-24T15:34:11Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+
+    private static string[] SplitIntoLines(string text)
+    {
+        return text.Split(['\n', '\r']);


Все-таки рекомендую использовать IDE полностью)

Не успел настроить Rider, а VS не дает такие подсказки=)

dmitgaranin · 2024-11-24T15:34:52Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+
+public class MarkdownTokenParser
+{
+    private readonly Dictionary<string, IHtmlTag> _markdownTags = new()


А правда, что MarkdownTokenParser ответственен за теги? Тут может помочь SRP
В этом классе есть и другие примеры наршения SRP

dmitgaranin · 2024-11-24T15:35:41Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+
+    private Token ParseWord(string word)
+    {
+        return TryParseMarkdownTag(word, out var token) 


Сплит по строкам - отлично, но сплит по пробелам без доп логики - это хорошо? Не будет ли в этом каких-то подводных камней?

Думаю, что сплит по пробелам лишний. Строку буду парсить на токены.

dmitgaranin · 2024-11-24T15:38:09Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+        if (matchingTag == null)
+            return false;
+
+        token = matchingTag.ToHtml(value);


matchingTag.ToHtml выглядит странно. Если он про конвертацию в html, то нарушается SRP
А если какое-то сопоставление с TokenType, то тоже не ок, т.к. нет такого md-тега как html

Или я не понял твою мысль

dmitgaranin · 2024-11-24T15:38:47Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+    private IHtmlTag? FindMatchingTag(string value)
+    {
+        return _markdownTags
+            .Where(tag => value.StartsWith(tag.Key))


Такие поиски точно закроют все-все потребности конвертации?

dmitgaranin · 2024-11-24T15:44:31Z

cs/Markdown/Html/BoldTag.cs

@@ -0,0 +1,11 @@
+using Markdown.Tokens;


Посмотрел решение в целом - пока не понимаю, будет ли оно работать вообще или нет. Подход со стороны "от общего к частному" - супер, но вот по текущему проектированию у меня не сложилось понимание, как будут закрываться на 2 и 3 балла задания

dmitgaranin · 2024-11-25T15:19:35Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+    {
+        for (var i = 0; i < line.Length; i++)
+        {
+            //TODO: ����� ����� ���������� ���������� ������ �� ������


А что здесь написано? Какой кодировкой? Давай все оформлять в стандартной - UTF-8

Здесь будет реализация парсинга строк на токены=)

dmitgaranin · 2024-11-25T15:32:36Z

cs/Markdown/Tokens/MarkdownTokenParser.cs

+            //TODO: ����� ����� ���������� ���������� ������ �� ������
+            var currStr = line.Substring(i, 1);
+
+            var isTagToken = MarkdownToken.TryGetTagToken(currStr, out var tagToken);


Раньше идея была работать с токенами абстрактно, сейчас, предполагаю, идея модернизировалась и теперь с текстом будет более плотное взаимодействие. К сожалению, я не нашел никаких сущностей, ответственных за md-теги, их обработку, логику и тд. Оттого есть подозрение, что логика их обработки будет или размазана, или храниться в одном классе потоком кода. Так делать точно не стоит) У нас ООП и DDD =) а еще всегда держим в голове SRP

dmitgaranin · 2024-11-25T15:39:01Z

cs/Markdown/Tokens/MarkdownToken.cs

+    public TokenType Type { get; } = type;
+    public bool IsClosedTag { get; } = isClosedTag;
+
+    public static bool TryGetTagToken(string str, out MarkdownToken resultToken)


Кстати) давай попробуем анемичные модели данных. Т.е. оставлять объекты описательными, а все методы над ними выносить в самостоятельные классы. Зачем? Объект и бизнес-требования могут быть связаны сегодня, но нет гарантий, что завтра не появится бизнес-требование v2 или v3, а объект почти наверняка останется тот же. Также можно рассмотреть через SRP-призму: в текущем классе две ответственности

описание объекта токена разметки

методы формирования токена на основе строки

И следует стараться оставлять в классе только одну причину для изменений

dmitgaranin · 2024-11-27T15:01:55Z

cs/Markdown/Markdown/Processing/MarkdownProcessor.cs

+            foreach (var handler in _tokenHandlers)
+            {
+                if (handler.CanHandle(tokenContext.CurrentToken) &&
+                    handler.Handle(tokenContext))


Возможно я не понял логику работы, но есть большое подозрение, что tokenContext будет изменяться внутри Handle. Если это действительно так, то предлагаю вспомнить комментарий из предыдущей задачи =)
kontur-courses/tdd#237 (comment) (он resolved, а гитхаб туповат, потому нужно явно развернуть комментарий...)

dmitgaranin · 2024-11-27T15:03:35Z

cs/Markdown/Markdown/Processing/MarkdownProcessor.cs

+        {
+            foreach (var handler in _tokenHandlers)
+            {
+                if (handler.CanHandle(tokenContext.CurrentToken) &&


Если у тебя предполагается всегда жесткая связка CanHandle && Handle, то предлагаю попробовать Try-Parse подход

dmitgaranin · 2024-11-27T15:05:46Z

cs/Markdown/Markdown/Processing/MarkdownProcessor.cs

+    public IList<MarkdownToken> Process(IList<MarkdownToken> tokens)
+    {
+        var tokenContext = new TokenProcessingContext(tokens);
+        var resultContextModifier = new TokenProcessingContextModifier(tokenContext);


Связка new TokenProcessingContextModifier(tokenContext); с var tokenContext = new TokenProcessingContext(tokens); выглядит "грязной" - данные меняются неявно. Такой подход стоит избегать, т.к. при работе с DI очень сильно может аукнуться) Да и в целом код, который stateless, сиииильно проще в поддержке, в отличии от stateful

Также могу прилинковать упомянутый выше коммент из прошлого ревью kontur-courses/tdd#237 (comment)

dmitgaranin · 2024-11-27T15:07:16Z

cs/Markdown/Markdown/Processing/MarkdownProcessor.cs

+
+namespace Markdown.Markdown.Processing;
+
+public class MarkdownProcessor : IMarkdownProcessor


Правда ли этот класс нужен? Кажется, его логику вполне комфортно унести в Md

dmitgaranin · 2024-11-27T15:10:09Z

cs/Markdown/Markdown/Handlers/EmphasisLevel.cs

@@ -0,0 +1,8 @@
+namespace Markdown.Markdown.Handlers;
+
+public enum EmphasisLevel


Насколько понимаю, Emphasis - про акцент и подчеркивание (в плане фокуса внимания) (словарь). Думаю, в данном контексте уместнее priority

dmitgaranin · 2024-11-27T15:13:03Z

cs/Markdown/Markdown/MarkdownTokenParser.cs

+            {
+                yield return token;
+            }
+            yield return MarkdownTokenCreator.CreateSymbolToken("\n");


В проекте много дублирования таких символов (\n и прочих)

dmitgaranin · 2024-11-27T15:16:56Z

cs/Markdown/Markdown/Processing/TokenProcessingContext.cs

+
+namespace Markdown.Markdown.Processing;
+
+public class TokenProcessingContext(IList<MarkdownToken> tokens) : IProcessingContext


Раз такие объекты появились, пора изучать и использовать records =)

dmitgaranin · 2024-11-27T15:17:12Z

cs/Markdown/Markdown/Processing/TokenProcessingContextModifier.cs

+
+namespace Markdown.Markdown.Processing;
+
+public class TokenProcessingContextModifier(IProcessingContext processingContext)


Этот класс правда нужен?

dmitgaranin · 2024-11-27T15:17:50Z

cs/Markdown/Markdown/Tokens/MarkdownTokenCreator.cs

+            " " => new MarkdownToken(content, TokenType.Space),
+            "_" => new MarkdownToken(content, TokenType.TagPart),
+            "#" => new MarkdownToken(content, TokenType.Header),
+            "\n" => new MarkdownToken(content, TokenType.NewLine),


В сети наверняка есть много подробностей о прокачанном pattern mathing в c#
В данном случае "\n" or "\r" => new MarkdownToken(content, TokenType.NewLine),

dmitgaranin · 2024-11-27T15:18:08Z

cs/Markdown/Md.cs

+    {
+        var tokens = _tokenParser
+            .Parse(text)
+            .ToArray();


А зачем финализация здесь?

dmitgaranin · 2024-12-09T14:55:07Z

cs/Markdown/Markdown/TextTokenizer.cs

+    {
+        var lines = SplitIntoLines(text);
+        var tokens = new List<IToken>();
+        for (var i = 0; i < lines.Length; i++)


Как будто можно просто через foreach

dmitgaranin · 2024-12-09T14:59:43Z

cs/Markdown/Markdown/Tokens/MarkdownTokenCreator.cs

+        {
+            MarkdownConstants.Space => new MarkdownToken(content, TokenType.Space),
+            MarkdownConstants.Escape => new MarkdownToken(content, TokenType.Escape),
+            _ => CreateTextToken(content)


А зачем? Насколько понял, после строки return token.Type != TokenType.Text; нам будет не важно на такой токен, получается, сделали работу просто так

dmitgaranin · 2024-12-09T15:04:26Z

cs/Markdown/Markdown/Tokens/MarkdownTokenCreator.cs

+
+    public static bool TryCreateTagToken(string content, out IToken token)
+    {
+        token = CreateTextToken(content);


Также вопрос про эту строку. Получается, у нас токен может быть заполнен текстом, хотя метод возвращает fase => снова сделали лишнюю работу и нагрузили heap

dmitgaranin · 2024-12-09T15:05:45Z

cs/Markdown/Markdown/TextTokenizer.cs

+    }
+
+    private static bool IsTokenEnded(string content, string symbol, TokenType tokenType) =>
+        (tokenType == TokenType.Text && (MarkdownTagValidator.IsTagStart(symbol) ||


Оооооочень сложно читать) давай пользоваться выносом в именованную функцию логики для упрощения чтения и понимания кода

dmitgaranin · 2024-12-09T15:39:10Z

cs/MarkdownTests/MdShould.cs

+    public string RenderHeaders_ShouldNotParseInvalidCases(string markdownText) =>
+        markdown.Render(markdownText);
+
+    //[TestCase("Markdown.md", TestName = "Convert Markdown file to HTML")]


Лишнее)

dmitgaranin · 2024-12-17T15:08:32Z

cs/Markdown/Markdown/Tokens/IToken.cs

+    bool IsCloseTag { get; }
+    TagType TagType { get; }
+    IToken? TagPair { get; }
+    Dictionary<AttributeType, IAttribute>? Attributes { get; }


Наличие этого свойства - еще один явный маркер, что архитектуру стоит перепроектировать)
По-хорошему, в наследниках интерфейсов должны использоваться все свойства, все методы и вообще, что есть в интерфейсе. Если где-то когда-то требуется использовать ? без обоснования со стороны бизнес-логики, а со стороны чтобы как-то поддержать расширение контрактов, то это анти-паттерн. Потому что теперь появляются куски логики, где почти везде нет атрибутов (у всех токенов, кроме картинки, он не используется), но приходится про них знать

Раньше это тоже было (TagPair), но он хотя бы покрывал больше одного сценария. В данном же случае - слишком костыльно выглядит)

dmitgaranin · 2024-12-17T15:08:55Z

cs/MarkdownTests/MarkdownTests.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="coverlet.collector" Version="6.0.0" />


Все еще есть лишние подключения)

dmitgaranin · 2024-12-17T15:10:01Z

cs/MarkdownTests/Resources/ExpectedResult_MarkdownSpec.html

+
+В Markdown разметке тег картинки обозначается следующим образом:
+
+![Alt text](URL)


А еще бы в основном файле спецификации это добавить, т.к. без этого не выполняется требование выполнения ДЗ

dmitgaranin · 2024-12-17T15:12:08Z

cs/MarkdownTests/MdShould.cs

+    public string RenderHeaders_NotParseInvalidCases(string markdownText) =>
+        markdown.Render(markdownText);
+
+    [TestCase(@"![Alt text](URL)", ExpectedResult = """<img src="URL" alt="Alt text">""",


Не хватает тесты на реальный url
А также на взаимодействие с другими тегами (полужирный, курсив, excape)

AlexxSaz added 7 commits November 21, 2024 20:18

Initial commit

ee8842e

Html tag classes created

f19c836

Class token created

cd5dc04

Token parser class created

be46adb

Renderer class created

79d1e65

Md class changed to use new classes

e149587

Created test for md class

f8e12c2

AlexxSaz force-pushed the master branch from 26a2a90 to f8e12c2 Compare November 24, 2024 15:13

dmitgaranin reviewed Nov 24, 2024

View reviewed changes

AlexxSaz added 4 commits November 25, 2024 19:52

Added tag converter class

dc503e0

Token parser reworked to SRP

aee3615

Added implementation to HtmlRenderer class

c17136a

Updated Md class

3bbe844

dmitgaranin reviewed Nov 25, 2024

View reviewed changes

AlexxSaz added 6 commits November 27, 2024 17:31

Added markdown token handlers

f1068d7

Added markdown token processor

fd6a1a0

Added markdown token creator

4a6be5d

Modified token parser

f0429ab

Rework main class with new classes

65119ec

Implemented markdown processor interface

64c2a3f

dmitgaranin reviewed Nov 27, 2024

View reviewed changes

AlexxSaz added 8 commits December 3, 2024 17:48

Few days work

e9f8459

Evening work

1867fad

Job work

0c49358

Emphasis handler reworked

1e36da5

Validation of underscore tags reworked

7955386

Added more validation of underscore tags

5090645

Refactored EmphasisHandler

6bf34a4

Ephasis handlers implementations added

4ed3d1f

AlexxSaz added 4 commits December 8, 2024 20:55

All logic implemented

288342e

Tokenizer refactored

4a64ed2

Hadlers refactored

e38afe3

More handlers refacting

a176465

dmitgaranin reviewed Dec 9, 2024

View reviewed changes

AlexxSaz added 22 commits December 11, 2024 22:12

TextInsideEmphasisHandler refactored

ff1b502

PairEmphasisHandler refactored

4a8f2b4

Refactored implementation by PR comments

498c754

DifferentWordsEmphasisHandler refactored

5d67ab8

SkipEmphasisHandler refactored

ee04ac4

IntersectEmphasisHandler refactored

f921258

NestedEmphasisHandler refactored

3fb6a79

Added SeriesEmphasisHandler

80fcee3

Added image tag tests

f8f5d84

Added attributes

93a160f

HtmlConverter reworked to convert image tag

954e5c0

HtmlConverter reworked to convert image tag

b66c900

HeaderHandler fix

d13d24a

Created new folder for underscore tag validators

751f4ff

Added validators for image tag

e455978

EmphasisHandlers fix

c59dd5e

Created handlers for image tag

1aadbc9

Updated main test

91dd8ad

Merge branch 'imageTag'

04644f3

HeaderHandler refactored

4de72d8

NonPairEmphasisTagHandler refactored

dacaa2c

UnionImageHandler refactored

4f087ad

dmitgaranin reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Сазонов Александр #233

Сазонов Александр #233

AlexxSaz commented Nov 24, 2024

dmitgaranin Nov 24, 2024

AlexxSaz Nov 24, 2024

dmitgaranin Nov 24, 2024

dmitgaranin Nov 24, 2024

AlexxSaz Nov 25, 2024

dmitgaranin Nov 24, 2024

dmitgaranin Nov 24, 2024

dmitgaranin Nov 24, 2024

dmitgaranin Nov 25, 2024

AlexxSaz Nov 25, 2024

dmitgaranin Nov 25, 2024

dmitgaranin Nov 25, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Nov 27, 2024

dmitgaranin Dec 9, 2024

dmitgaranin Dec 9, 2024

dmitgaranin Dec 9, 2024

dmitgaranin Dec 9, 2024

dmitgaranin Dec 9, 2024

dmitgaranin Dec 17, 2024

dmitgaranin Dec 17, 2024

dmitgaranin Dec 17, 2024

dmitgaranin Dec 17, 2024


		namespace Markdown.Markdown.Processing;

		public class MarkdownProcessor : IMarkdownProcessor

		@@ -0,0 +1,8 @@
		namespace Markdown.Markdown.Handlers;

		public enum EmphasisLevel


		namespace Markdown.Markdown.Processing;

		public class TokenProcessingContext(IList<MarkdownToken> tokens) : IProcessingContext


		namespace Markdown.Markdown.Processing;

		public class TokenProcessingContextModifier(IProcessingContext processingContext)


		В Markdown разметке тег картинки обозначается следующим образом:

		![Alt text](URL)

Сазонов Александр #233

Are you sure you want to change the base?

Сазонов Александр #233

Conversation

AlexxSaz 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

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