Skip to content
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

SPT-1998 Скрытие AsyncStream и исправление опечаток (6) #138

Merged
merged 2 commits into from
May 24, 2024

Conversation

mrandrewsmith
Copy link
Collaborator

Что сделано

  • Убрал public с элементов, содержащих AsyncStream.
  • Убрал мердж для ноды.
  • Поправил опечатки.
  • Переименовал LoggableNode в Node (Помню было замечание, забыл переименовать)

@mrandrewsmith mrandrewsmith self-assigned this May 13, 2024
@mrandrewsmith mrandrewsmith changed the title SPT-1998 Скрытие AsyncStream и исправление опечаток SPT-1998 Скрытие AsyncStream и исправление опечаток (6) May 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.32%. Comparing base (09fdbc8) to head (de8002c).

Additional details and impacted files
@@                         Coverage Diff                         @@
##           SPT-1998-fix-migration-problems     #138      +/-   ##
===================================================================
+ Coverage                            91.41%   92.32%   +0.91%     
===================================================================
  Files                                   87       86       -1     
  Lines                                 1351     1342       -9     
===================================================================
+ Hits                                  1235     1239       +4     
+ Misses                                 116      103      -13     
Flag Coverage Δ
tests 92.32% <100.00%> (+0.91%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@NullIsOne NullIsOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Небольшая опечатка в комменте, а так норм

/// Возвращает имя типа строкой
var objectName: String {
return "\(type(of: self))"
}

/// Имея обхекта в формате:
/// Имея объекта в формате:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Имея объекта в формате:
/// Имя объекта в формате:

/// Этот узел реализует объединение двух узлов в один узел, поддерживающий работу с потоком данных.
/// Первым вызывает process у первого переданного узла, затем у второго.
/// В общем случае слушатель может быть оповещен дважды.
open class MergedAsyncStreamNode<Input, Output>: AsyncStreamNode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а, то есть и это решили с корнями выпилить? я прост не помню о чем договаривались уже - казалось, что просто должны были internal сделать как и AsyncStreamNode

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Просто в любом случае надо было удалять метод, который превращает AsyncNode в MergedAsyncStreamNode из-за замены модификатора доступа. Поэтому я решил заодно этот класс удалить, все равно под него тестов нет + логики особо нет. Это была эксперементальная штука, которая не понятно нужна или нет.

@mrandrewsmith mrandrewsmith changed the base branch from SPT-1998-fix-migration-problems to 5.0.0 May 24, 2024 11:14
@mrandrewsmith mrandrewsmith merged commit c549019 into 5.0.0 May 24, 2024
2 checks passed
@mrandrewsmith mrandrewsmith deleted the SPT-1998-hide-async-stream branch June 13, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants