-
Notifications
You must be signed in to change notification settings - Fork 320
Якшибаев Данил #255
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?
Якшибаев Данил #255
Conversation
|
|
||
| public static class TagRenderer | ||
| { | ||
| public static string WrapEm(string content) => $"<em>{content}</em>"; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/TokensUtils/Token.cs
Outdated
| public class Token | ||
| { | ||
| public string Value { get; set; } | ||
| public string TokenType { get; set; } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Остался вопрос с наименованием
| { | ||
| public string RenderLine(string line) | ||
| { | ||
| //рендерит по определенным правилам строку, будут добавлены методы для рендеринга по определенным символам |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
токенайзер будет проходить по строке и разбивать ее на токены
а рендерлайн будет брать эти токены и использовать их, чтобы собрать html
|
|
||
| public class RenderFragment : IRenderFragment | ||
| { | ||
| public string RenderLine(string line) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Логически они выполняют разную суть. RenderFragment будет рендерить по определенным правилам объекты, например, в будущем могут добавиться методы для рендера блока. А RenderText будет рендерить весь текст, т.е. проходить по каждой строке и применять определенный рендер. Лучше наверно разделить на три метода и оставить в одном интерфейсе и одном классе
…кторинг в классе Token.cs
… с обработкой _ и __
…вина тестов падают, т.к. логику не до конца реализована
cs/Markdown/Dto/Tag.cs
Outdated
|
|
||
| namespace Markdown.Dto; | ||
|
|
||
| public class Tag |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Dto/Token.cs
Outdated
| @@ -0,0 +1,13 @@ | |||
| namespace Markdown.Dto; | |||
|
|
|||
| public class Token | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -0,0 +1,11 @@ | |||
| namespace Markdown.Dto; | |||
|
|
|||
| public enum TokenType | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| { | ||
| public static IEnumerable<string> EnumerateLines(this string? markdown) | ||
| { | ||
| if (markdown is null) yield break; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
- Смотри другие подобные места
| var tokens = tokenizer.Tokenize(line).ToList(); | ||
| var result = new StringBuilder(); | ||
|
|
||
| var tagStack = new Stack<Tag>(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| private void ProcessHeader(Stack<Tag> tagStack) | ||
| { | ||
| tagStack.Push(new Tag(TokenType.Header, "#", new StringBuilder())); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| while (tagStack.Count > 0) | ||
| { | ||
| var top = tagStack.Pop(); | ||
| string rendered; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.Tests; | ||
|
|
||
| public class MarkdownTests |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Да, тесты падают, т.к. пока не вся логика рендера сделана.
Тесты переделаю наверно с генерилкой позже, как в предыдущей домашке
И добавлю новые тесты в том числе
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.
Посмотрел тесты. Тест на производительность у меня не проходит. Давай немного изменим подход? Подойдем к этому как к замеру сложности алгоритма. У нас есть длина входного слова и выходное время (примерное). Как мы можем примерно оценить сложность исходя из этих данных? Как поймем, что для 10_000 элементов результат в <1сек. приемлем?
cs/Markdown/Tests/MarkdownTests.cs
Outdated
| { | ||
| private IRender render = new Render(); | ||
|
|
||
| [Test] |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Tests/MarkdownTests.cs
Outdated
| private IRender render = new Render(); | ||
|
|
||
| [Test] | ||
| public void Markdown_ShouldBeTextWrappedWithEmTag_WhenSurroundedBySingleUnderscores_Test() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Tests/MarkdownTests.cs
Outdated
| result.Length.Should().BeGreaterThanOrEqualTo(longLine.Length); | ||
|
|
||
| var stopwatch = Stopwatch.StartNew(); | ||
| render.RenderText(longLine); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
- Посмотри в сторону AAA паттерна (Act, Arrange, Assert)
cs/Markdown/Tests/MarkdownTests.cs
Outdated
|
|
||
| var result = render.RenderText(longLine); | ||
|
|
||
| result.Length.Should().BeGreaterThanOrEqualTo(longLine.Length); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| switch (currentChar) | ||
| { | ||
| case '\\': |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/TokensUtils/TagRender.cs
Outdated
| public static string Wrap(TokenType type, string content) | ||
| => type switch | ||
| { | ||
| TokenType.Italic => $"<em>{content}</em>", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…мапинг для специальных символов и немного изменил Tokenizer.cs, в связи с изменениями с Token.cs
…реписал тесты на testsource и добавил новый тест.
| { | ||
| var parent = tagsStack.Count > 0 ? tagsStack.Peek() : null; | ||
|
|
||
| if (token.CanClose && tagsStack.Count > 0 && !IsInvalidCloseContext(parent, token, 2)) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Забыл ответить, тег родительский так, как у они могут идти в перемешку и нужно знать родительский тег, чтобы эту логику соблюдать, например _Вот т_ак. И у первого _ будет родительский __
|
|
||
| public class Tokenizer : ITokenizer | ||
| { | ||
| private static readonly HashSet<char> SpecialSymbols = new() { '\\', '_', '#' }; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Dto/Token.cs
Outdated
| @@ -0,0 +1,3 @@ | |||
| namespace Markdown.Dto; | |||
|
|
|||
| public record Token(string Value, TokenType Type, bool CanOpen, bool CanClose, bool InsideWord); No newline at end of file | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
- По поводу InsideWord. Как насчет добавить Position из Begin, Inside и End?
….cs не record классами с логикой. Благодаря логике в Tag.cs рендер работает быыстрее, т.к. не нужно делать долгие проверки на decimal.tryparse и добавление контента
… чтобы следовать SRP
…сколько замеров времени на разное количество строк
cs/Markdown/Tests/MarkdownTests.cs
Outdated
| totalTime += sw.Elapsed.TotalMilliseconds; | ||
| } | ||
|
|
||
| var avgTime = totalTime / runsPerSize; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| namespace Markdown.TagUtils.Abstractions; | ||
|
|
||
| public interface ITagManager |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| public interface ITagContentManager | ||
| { | ||
| public void AppendToParentOrResult(Stack<Tag> tagsStack, StringBuilder result, string content); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| { | ||
| var value = isStrong ? "__" : "_"; | ||
| var type = isStrong ? TokenType.Strong : TokenType.Italic; | ||
| var prevChar = index > 0 ? line[index - 1] : (char?)null; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cs/Markdown/Dto/Token.cs
Outdated
| (true, true) => TokenPosition.Inside, | ||
| (true, false) => TokenPosition.Begin, | ||
| (false, true) => TokenPosition.End, | ||
| (false, false) => TokenPosition.None |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…le, исправил замечания по тесту
… и классы наследники.
@Luvr681