Implementation of Async/Await Concurrency #4
Conversation
1. В блоке "Аналоги" в карточке товара должен быть зеленый текст (посмотреть в фигме); 2. В блоке "Вы недавно смотрели" в карточке товара должен быть зеленый текст (посмотреть в фигме); 3. Сделать порядок блоков в карточке товара согласно фигме; 4. Адреса аптек не по фигме. Вопрос насколько трудоемко сделать по фигме (в андроиде реализовано как надо); 5. В моих заказах при переходе в конкретный заказ внизу где отображаются товары нет инфы по кол-ву (сколько штук) и еще доп инфы;
DocC added Test expanded
+ Test project
UI refactored
|
@NSFuntik thx for contribution, quite a lot to comprehend. I'll review it once have some free time, thx for understanding. |
illabo
left a comment
illabo
left a comment
There was a problem hiding this comment.
I want to thank you again for immense amount of work you offered. Adding name suggestions is nice addition to the functionality of the package. The UI also looks useful. However the whole PR might benefit from separation into isolated changes. If you okay to do so I'd like to ask you to find the possibility to split it into separate PRs for:
- Adding name suggestions.
- Introducing async/awaits.
- Improving the tests and structure.
I understand you might be unwilling to do an extra job and I have to be grateful for effort you did yet it is objectively hard to comprehend all the changes and how different new features work together reviewing such a big PR.
Please let me know your opinion regarding the comments I left below. It might and should be discussed I believe.
Thank you again for the PR.
| import IIDadata | ||
| import IIDadataUI | ||
|
|
||
| struct ExampleView: View { |
There was a problem hiding this comment.
I'm puzzled why to have multiple examples here and in IIDadata directory. Couldn't these files be joined in a single place?
| import protocol Combine.ObservableObject | ||
| import Foundation | ||
|
|
||
| @available(iOS 14.0, *) |
There was a problem hiding this comment.
While I respect the strive to use the newer technologies I'm not sure it is really beneficial to lock in the functionality only on newer versions of iOS. Yes, I'm aware it is the time of iOS 18 now but still i can see few problems here:
- macOS compatibility is left behind.
- if e.g. I revive some old project using this lib some effort to be applied to make the code buildable (this point also applies to some concerns I would voice below).
- I see what you did here. Actor however is a reference type only executes one operation at a time. Wouldn't it be better to keep it class which could fetch multiple requests in parallel (I don't have a strong suggestion here, but
fetchResponsemethod doesn't access any class variables so no risk of potential race access is present. - The library consumer is forced to use async/await to use it. There's much more alternatives to async/awaits. To name just few widely used: classic closures approach, publishers.
Here I might suggest to create an actor to be a facade to the class. It would allow us to benefit from maintaining the backward compatibility and to have async/awaits. This separate actor could have the very same @available annotation as this one I'm criticising now.
| /// May throw if request is timed out. | ||
| public init(api: String /* , checkWithTimeout timeout: Int */ ) throws { | ||
| self.init(apiKey: api) | ||
| Task { try await checkAPIConnectivity() } |
There was a problem hiding this comment.
I'd suggest the same hygiene we always had with the captures in closures.
Task { [weak self] in try await self?.checkAPIConnectivity() }If the instance of type is dropped before this check is finished there going to be the leak because of hard reference.
| /// | ||
| /// - Parameter query: Query string to send to API. String of a free-form e.g. address part. | ||
| /// - Returns:``AddressSuggestionResponse`` - result of address suggestion query. | ||
| public func suggestAddress(_ query: String) async throws -> AddressSuggestionResponse { |
There was a problem hiding this comment.
As I voiced the concern before converting all the methods to be async doesn't look nice to library consumer. Could we please move these async methods to a separate type?
| // MARK: - AddressQueryConstraint | ||
| /// AddressQueryConstraint used to limit search results according to | ||
| /// [Dadata online API documentation](https://confluence.hflabs.ru/pages/viewpage.action?pageId=204669108). | ||
| @available(iOS 14.0, *) |
There was a problem hiding this comment.
Why it should be checked for iOS version here?
|
|
||
| import Foundation | ||
|
|
||
| public extension FioSuggestion { |
There was a problem hiding this comment.
Thank you for extending the supported types of suggestions.
| // MARK: - IIDadataSuggestsPopover.SuggestionsPopover | ||
|
|
||
| @available(iOS 15.0, *) | ||
| public extension IIDadataSuggestsPopover { |
There was a problem hiding this comment.
It is a great addition to be used with Dadata suggestions provided in this library but I hate to express the doubt it should be an integral part of this project. It might be a great library itself having IIDadata as a dependency. While I appreciate your effort and thank you for offering this code the library was intended to be as bare bones as it is possible. I want its consumers to make theirs own decisions whether the are deciding on a way to display the suggestions in theirs UI or using the suggestions API without displaying it unmodified to the user.
Bringing the UI code into such an utility library would force this UI on the user. What if in theirs app they want to display suggestions completely different?
2 participants
Refactored Implementation of Async/Await Concurrency
Also:
– Added FIO suggestions functionality
– SwiftUI Disclosure Popup view modifier to easy installation and integration process