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.

Sign up for GitHub

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

Jump to bottom

SPT-1998 Удаление старого подхода #126

Merged
merged 6 commits into from
Apr 24, 2024
Merged

SPT-1998 Удаление старого подхода #126

merged 6 commits into from
Apr 24, 2024

Conversation

mrandrewsmith
Copy link
Collaborator

@mrandrewsmith mrandrewsmith commented Apr 10, 2024 •
edited
Loading

Что сделано

  • Удалены legacy методы и Observer с Context
  • Third Party вынесен в отдельный локальный пакет
  • Удалена нода ChainConfiguratorNode и диспатчи в нодах, из-за ненадобности
  • Удалена зависимость CoreEvents

@mrandrewsmith mrandrewsmith self-assigned this Apr 10, 2024
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-legacy-remove branch from 180a8ea to f9e94d3 Compare April 10, 2024 08:00
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-part2 branch 2 times, most recently from dd62b3b to 5c8ac32 Compare April 10, 2024 13:04
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-legacy-remove branch 2 times, most recently from e375731 to cef626e Compare April 10, 2024 13:12
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 79.98%. Comparing base (7604cdf) to head (66df030).
Report is 5 commits behind head on 5.0.0.

Files Patch % Lines
NodeKit/Chains/UrlChainsBuilder.swift 0.00% 14 Missing ⚠️
NodeKit/Chains/UrlServiceChainBuilder.swift 0.00% 5 Missing ⚠️
...t/Layers/Utils/AccessSafe/TokenRefresherNode.swift 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##            5.0.0     #126       +/-   ##
===========================================
+ Coverage   56.44%   79.98%   +23.54%     
===========================================
  Files          96       82       -14     
  Lines        2562     1259     -1303     
===========================================
- Hits         1446     1007      -439     
+ Misses       1116      252      -864
Flag Coverage Δ
tests 79.98% <25.00%> (+23.54%) ⬆️

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.

@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-part2 branch from 5c8ac32 to 729253a Compare April 10, 2024 14:15
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-legacy-remove branch from cef626e to 059ee22 Compare April 10, 2024 14:15
NullIsOne
NullIsOne reviewed Apr 12, 2024
View reviewed changes
NodeKit/Layers/RequestProcessingLayer/RequestSenderNode.swift Outdated Show resolved Hide resolved
NodeKit/Layers/RequestProcessingLayer/RequestSenderNode.swift
@@ -26,62 +26,23 @@ open class RequestSenderNode<Type>: AsyncNode, Aborter {

/// Менеджер сессий
private(set) var manager: URLSession

private var responseQueue: DispatchQueue
Copy link
Contributor

Choose a reason for hiding this comment

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

А как теперь потоком управлять, если понадобится вызвать из одного, а ответ получить в другом?
Если что, нам не всегда нужен результат в main. Иногда запросы выполняются фоном.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Про управление потоками еще отпишу в доке, но если в кратце, то снаружи управлять потоком в котором будет работать метод ноды нет смысла, так как все методы process внутри ноды будут выполнятся в рандомном потоке. А ответ на main очередь можно задиспатчить в сервесе или с помощью combine.

Package.swift Show resolved Hide resolved
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-part2 branch from 729253a to 7604cdf Compare April 16, 2024 12:15
@mrandrewsmith mrandrewsmith force-pushed the SPT-1998-unit-tests-legacy-remove branch from 059ee22 to 3cb3c20 Compare April 16, 2024 12:17
@mrandrewsmith mrandrewsmith requested a review from NullIsOne April 16, 2024 12:25
chausovSurfStudio
chausovSurfStudio approved these changes Apr 19, 2024
View reviewed changes
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.

мелкие замечания, в целом оч круто после чистки)

NodeKit/CacheNode/UrlCacheReaderNode.swift Show resolved Hide resolved
NodeKit/Core/Node/LoggableNode.swift

/// Протокол, описывающий любой узел или цепочку узлов.
/// Необходим для объединения всех типов узлов и добавления общих методов.
public protocol Node { }
Copy link
Contributor

Choose a reason for hiding this comment

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

а почему название файла LoggableNode?

ну то есть окей, тут есть extension, в котором есть вещи для работы с логами, но почему не просто Node.swift?) все же он тут главный (ну как главный - теперь уже не сильно главный после урезани), он тут объявлен, от него много чего наследуется...

ну так, на подумать вброс)

NodeKit/Layers/Utils/AccessSafe/TokenRefresherNode.swift Outdated Show resolved Hide resolved
NodeKit/Utils/AsyncIterator/AsyncIterator.swift Show resolved Hide resolved
@mrandrewsmith mrandrewsmith changed the base branch from SPT-1998-unit-tests-part2 to 5.0.0 April 24, 2024 09:05
@mrandrewsmith mrandrewsmith merged commit 915d6f0 into 5.0.0 Apr 24, 2024
1 check passed
@mrandrewsmith mrandrewsmith deleted the SPT-1998-unit-tests-legacy-remove branch May 7, 2024 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NullIsOne NullIsOne NullIsOne approved these changes

@chausovSurfStudio chausovSurfStudio chausovSurfStudio approved these changes

@Goldilocks97 Goldilocks97 Awaiting requested review from Goldilocks97

Assignees

@mrandrewsmith mrandrewsmith

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mrandrewsmith @codecov-commenter @chausovSurfStudio @NullIsOne