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 Мелкий рефакторинг + моки на билдер (1) #133

Merged
merged 7 commits into from
May 24, 2024

Conversation

mrandrewsmith
Copy link
Collaborator

@mrandrewsmith mrandrewsmith commented May 2, 2024

Что сделано

  • Добавлены структуры-обертки AnyAsyncNode и AnyAsyncStreamNode в связи с тем, что компилятор не может правильно зарезолвить типы при использование any AsyncNode в билдере. При этом использовать some AsyncNode мы не можем, так как some не поддерживается в протоколах и нельзя наследовать методы класса, которые возвращают some.
  • Добавлены методы eraseToAnyNode, которые оборачивают ноду в AnyAsyncNode или AnyAsyncStreamNode.
  • Не много изменен билдер.
  • Написаны моки на билдеры. Теперь можно передавать в сервис протокол ChainBuilder, а в тестах менять на мок и стаббать ноды.
  • Переименованы классы, методы и переменные с Url на URL.
  • Удалена нода LoadIndicatableNode (по моему бесползеная).
  • Удалены лишние параметры из моделей (serializationTimeout и metrics)

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2024

Codecov Report

Attention: Patch coverage is 65.88785% with 73 lines in your changes are missing coverage. Please review.

Project coverage is 91.50%. Comparing base (da998b5) to head (a9b9219).
Report is 1 commits behind head on 5.0.0.

Files Patch % Lines
NodeKit/NodeKit/Chains/ChainBuilder.swift 70.70% 29 Missing ⚠️
NodeKit/NodeKit/Chains/ServiceChainProvider.swift 30.76% 27 Missing ⚠️
...odeKit/Core/Node/Async/MergedAsyncStreamNode.swift 0.00% 11 Missing ⚠️
NodeKit/NodeKit/Core/Node/Async/AsyncNode.swift 50.00% 2 Missing ⚠️
NodeKit/NodeKit/Encodings/ParameterEncoding.swift 75.00% 2 Missing ⚠️
...rs/RequestBuildingLayer/URLQueryInjectorNode.swift 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            5.0.0     #133      +/-   ##
==========================================
+ Coverage   91.39%   91.50%   +0.11%     
==========================================
  Files          83       85       +2     
  Lines        1267     1260       -7     
==========================================
- Hits         1158     1153       -5     
+ Misses        109      107       -2     
Flag Coverage Δ
tests 91.50% <65.88%> (+0.11%) ⬆️

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.

Немного вопросов

NodeKit/NodeKit/Chains/ChainBuilder.swift Show resolved Hide resolved
NodeKit/NodeKit/Chains/ServiceChainProvider.swift Outdated Show resolved Hide resolved
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-example-refresh branch from 5804141 to da998b5 Compare May 7, 2024 04:28
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-refactor-builders-and-add-mocks branch from dee8cc5 to 332afd4 Compare May 7, 2024 04:30
@mrandrewsmith
Copy link
Collaborator Author

Добавлен метод merge, для двух AsyncNode. Позволяет смержить две AsyncNode с одинаковыми Input и Output в AsyncStreamNode.

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.

Пока не попробовал метод с MergedNode для создания цепочки, но по основному содержанию в ПР могу поставить аппрув.

open class FirstCachePolicyNode: AsyncStreamNode {
// MARK: - Nested

/// Тип для читающего из URL кэша узла
public typealias CacheReaderNode = AsyncNode<UrlNetworkRequest, Json>
public typealias CacheReaderNode = AsyncNode<URLNetworkRequest, Json>
Copy link
Contributor

Choose a reason for hiding this comment

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

С новымMergedAsyncStreamNode эту ноду можно упразднить и встраивать кэширование как mergedNode где первый это CacheReader, а второй это NextProcessor.

@mrandrewsmith mrandrewsmith changed the base branch from SPT-1998-example-refresh to 5.0.0 May 7, 2024 13:53
@mrandrewsmith mrandrewsmith changed the title SPT-1998 Мелкий рефакторинг + моки на билдер SPT-1998 Мелкий рефакторинг + моки на билдер (1) May 13, 2024
@mrandrewsmith mrandrewsmith requested a review from NullIsOne May 13, 2024 12:40
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.

Проверил моки chain после правок.
Теперь они работают и не падают.

Copy link
Contributor

@chausovSurfStudio chausovSurfStudio left a comment

Choose a reason for hiding this comment

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

тесты и моки честно пролистал - надеюсь на вас с Никитой)

public var needsToThrowError: Bool

public init(needsToThrowError: Bool) {
self.needsToThrowError = needsToThrowError
Copy link
Contributor

Choose a reason for hiding this comment

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

а зачем оно добавилось, если никак не используется?

}

/// Время, затраченное на сериализацию овтета.
public var serializationDuration: TimeInterval {
Copy link
Contributor

Choose a reason for hiding this comment

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

аа, понял, таинственные nil и -1! эх, задумка хорошая была, надо запомнить на будущее, если вдруг какие-то метрики по проектам начнем собирать, в том числе и по работе с сервис слоем

@@ -1,13 +1,13 @@
import Foundation

/// Модель данных для конфигурироания цепочки преобразований для запроса в сеть.
public struct UrlChainConfigModel {
public struct URLChainConfigModel {
Copy link
Contributor

Choose a reason for hiding this comment

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

немного неожиданный вопрос - а эта структура вообще где-то используется?) а то я что-то кроме объявления и тестов больше её нигде не вижу...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@mrandrewsmith mrandrewsmith merged commit 3c802d7 into 5.0.0 May 24, 2024
1 check passed
@mrandrewsmith mrandrewsmith deleted the SPT-1998-refactor-builders-and-add-mocks 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