Package re-structure #280

tonikocjan · 2023-05-08T08:58:35Z

Re-structured package:
- PovioKitCore
  - Extensions/
- PKNetworking
  - ANC/
- PKPromiseKit
  - Promise/
- PKSwiftUI
  - Extensions/
  - ActionButton/
  - ProfileImageView/
- PKUIKit
  - DynamicCollectionCell/
  - GradientView/
  - PaddingLabel
  - TextField
- PKUtilities
  - a lot of stuff
Support for MacOS

borut-t

I like the new structure. The core is now small and lean 💪

Package.swift

Sources/Utilities/InAppPurchase/InAppPurchaseService.swift

borut-t · 2023-05-12T06:25:32Z

Sources/Core/Extensions/Foundation/Data+PovioKit.swift

-    do {
-      return try decoder.decode(type, from: self)
-    } catch let decodingError as DecodingError {
-      switch decodingError {
-      case .typeMismatch(_, let context),
-           .valueNotFound(_, let context),
-           .keyNotFound(_, let context),
-           .dataCorrupted(let context):
-        Logger.debug("Decoding (failure): \(type.self)", params: ["error": context.debugDescription])
-      @unknown default:
-        Logger.debug("Decoding (failure): \(type.self)", params: ["error": "Unknown decoding error."])
-      }
-      throw decodingError
-    } catch {
-      Logger.debug("Decoding (failure): \(type.self)", params: ["error": error.localizedDescription])
-      throw error
-    }


Why removed?

Logger is part of the Utilities package, do not want Core to depend on the subpackage.

Logging should be done on the client side.

Don't agree with both points.

True, but since the Logger is really the core of the whole package, I would leave it in the Core package.

That should be part of the library. Client doesn't and shouldn't know about the implementation details.

That should be part of the library. Client doesn't and shouldn't know about the implementation details.

I disagree. A simple extension function such as this one should be completely pure, i.e. it should not modify the global state.

I still tend to disagree. Extension is here for a reason, to "extend" the core functionality. By slashing pretty much all the code, the code left is meaningless, no gain for the developer. We have two options:

revert the change

remove the method completely

The function itself is fine, it extends core functionality to use your lingo. But I would not force upon the user as to how they want to handle (and log) the errors.
If it comes to this I will just remove the method.

borut-t · 2023-05-12T06:25:49Z

Sources/Core/Extensions/Foundation/Encodable+PovioKit.swift

-  func encode(with encoder: JSONEncoder) throws -> [String: Any] {
-    do {
-      let data = try encoder.encode(self)
-      guard let jsonObject = try JSONSerialization.jsonObject(with: data, options: []) as? [String: Any] else {
-        throw JSONError.serialization
-      }
-      return jsonObject
-    } catch let encodingError as EncodingError {
-      switch encodingError {
-      case .invalidValue(_, let context):
-        Logger.debug("Encoding (failure): \(type(of: self))", params: ["error": context.debugDescription])
-      @unknown default:
-        Logger.debug("Encoding (failure): \(type(of: self))", params: ["error": "Unknown encoding error."])
-      }
-      throw encodingError
-    } catch {
-      Logger.debug("Encoding (failure): \(type(of: self))", params: ["error": error.self])
-      throw error


Why removed?

Same as above.

Same here. Either revert the change or remove the method completely.

Package.swift

borut-t · 2023-06-30T20:35:08Z

Sources/Core/Extensions/Foundation/Data+PovioKit.swift

-    do {
-      return try decoder.decode(type, from: self)
-    } catch let decodingError as DecodingError {
-      switch decodingError {
-      case .typeMismatch(_, let context),
-           .valueNotFound(_, let context),
-           .keyNotFound(_, let context),
-           .dataCorrupted(let context):
-        Logger.debug("Decoding (failure): \(type.self)", params: ["error": context.debugDescription])
-      @unknown default:
-        Logger.debug("Decoding (failure): \(type.self)", params: ["error": "Unknown decoding error."])
-      }
-      throw decodingError
-    } catch {
-      Logger.debug("Decoding (failure): \(type.self)", params: ["error": error.localizedDescription])
-      throw error
-    }


Don't agree with both points.

True, but since the Logger is really the core of the whole package, I would leave it in the Core package.

That should be part of the library. Client doesn't and shouldn't know about the implementation details.

Sources/Utilities/InAppPurchase/InAppPurchaseService.swift

Sources/SwiftUI/ProfileImageView/ProfileImageView.swift

borut-t

Great work. Although I have a few concerns:

PR should re-structure the package, but is also doing other things
PR is adding support for macos
PR is changing existing code

We are doing three things in one PR which should be split into three separate PRs. I'm ok with first two points above to be done here, but let's scrap the third out into a separate PR.

borut-t · 2024-01-12T06:52:16Z

Sources/Core/Extensions/Foundation/Data+PovioKit.swift

-    do {
-      return try decoder.decode(type, from: self)
-    } catch let decodingError as DecodingError {
-      switch decodingError {
-      case .typeMismatch(_, let context),
-           .valueNotFound(_, let context),
-           .keyNotFound(_, let context),
-           .dataCorrupted(let context):
-        Logger.debug("Decoding (failure): \(type.self)", params: ["error": context.debugDescription])
-      @unknown default:
-        Logger.debug("Decoding (failure): \(type.self)", params: ["error": "Unknown decoding error."])
-      }
-      throw decodingError
-    } catch {
-      Logger.debug("Decoding (failure): \(type.self)", params: ["error": error.localizedDescription])
-      throw error
-    }


I still tend to disagree. Extension is here for a reason, to "extend" the core functionality. By slashing pretty much all the code, the code left is meaningless, no gain for the developer. We have two options:

revert the change

remove the method completely

borut-t · 2024-01-12T06:52:57Z

Sources/Core/Extensions/Foundation/Encodable+PovioKit.swift

-  func encode(with encoder: JSONEncoder) throws -> [String: Any] {
-    do {
-      let data = try encoder.encode(self)
-      guard let jsonObject = try JSONSerialization.jsonObject(with: data, options: []) as? [String: Any] else {
-        throw JSONError.serialization
-      }
-      return jsonObject
-    } catch let encodingError as EncodingError {
-      switch encodingError {
-      case .invalidValue(_, let context):
-        Logger.debug("Encoding (failure): \(type(of: self))", params: ["error": context.debugDescription])
-      @unknown default:
-        Logger.debug("Encoding (failure): \(type(of: self))", params: ["error": "Unknown encoding error."])
-      }
-      throw encodingError
-    } catch {
-      Logger.debug("Encoding (failure): \(type(of: self))", params: ["error": error.self])
-      throw error


Same here. Either revert the change or remove the method completely.

Tests/Tests/Core/Utilities/Exif/ExifTests.swift

borut-t · 2024-01-26T11:43:03Z

Great work @tonikocjan. We need some additional changes:

rebase branch
move /Core/Utilities/MediaPlayer to Utilities subpackage
move /Utilities/Logger to Core subpackage (it's a must on every project, even if we don't need Utilities subpackage I should have access to the Logger from the Core subpackage)
Async subpackage feels redundant to me. We have async code in other places as well, but we didn't move it here.

tonikocjan · 2024-02-09T11:14:19Z

Async subpackage feels redundant to me.

Apple also has a separate swift package for Async data structures and algorithms. I think separating makes sense as a user might only need some async algorithm that we provide.

We have async code in other places as well, but we didn't move it here.

The other async code is already nicely contained in the Networking and Promise sub-packages. Any other code that strongly utilizes Swift concurrency, should be moved inside the Async package.

Revert promise changes +1

borut-t · 2024-02-09T14:12:45Z

Async subpackage feels redundant to me.

Apple also has a separate swift package for Async data structures and algorithms. I think separating makes sense as a user might only need some async algorithm that we provide.

We have async code in other places as well, but we didn't move it here.

The other async code is already nicely contained in the Networking and Promise sub-packages. Any other code that strongly utilizes Swift concurrency, should be moved inside the Async package.

Great. Let's proceed 👌

borut-t

🙌

tonikocjan requested a review from a team May 8, 2023 08:58

tonikocjan added the needs review label May 8, 2023

borut-t reviewed May 12, 2023

View reviewed changes

borut-t reviewed Jun 30, 2023

View reviewed changes

borut-t added requested changes and removed needs review labels Aug 22, 2023

borut-t assigned tonikocjan Aug 22, 2023

tonikocjan added needs review and removed requested changes labels Oct 20, 2023

tonikocjan requested a review from borut-t October 20, 2023 07:06

tonikocjan removed their assignment Oct 20, 2023

borut-t assigned tonikocjan Jan 9, 2024

tonikocjan force-pushed the macos branch from 7289a0d to c595758 Compare January 11, 2024 12:16

tonikocjan removed their assignment Jan 11, 2024

borut-t requested changes Jan 12, 2024

View reviewed changes

borut-t added requested changes and removed needs review labels Jan 12, 2024

borut-t requested a review from a team January 12, 2024 07:00

tonikocjan requested a review from borut-t January 12, 2024 08:11

tonikocjan added needs review and removed requested changes labels Jan 12, 2024

tonikocjan added 5 commits February 9, 2024 12:04

UIKit for iOS only

e605e1d

Structure, MacOs

d98d5e3

Addressed PR comments

efd61d5

Fixes

b8797a4

Remove redundant @testable(s)

8a23040

Revert method param alignment

382adc0

Revert promise changes +1

tonikocjan force-pushed the macos branch from 01f9b21 to 382adc0 Compare February 9, 2024 11:15

tonikocjan requested review from a team and removed request for a team February 9, 2024 11:15

toJSON changes

e466975

borut-t approved these changes Feb 9, 2024

View reviewed changes

borut-t added merge needs review and removed needs review merge labels Feb 9, 2024

borut-t requested review from dejanskledar and a team February 9, 2024 14:13

borut-t added merge and removed needs review labels Feb 10, 2024

borut-t merged commit 7c1873c into develop Feb 10, 2024
1 check passed

borut-t deleted the macos branch February 10, 2024 08:40

borut-t pushed a commit that referenced this pull request May 16, 2024

Package re-structure (#280)

fab37b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package re-structure #280

Package re-structure #280

tonikocjan commented May 8, 2023 •

edited

Loading

borut-t left a comment

borut-t May 12, 2023

tonikocjan May 12, 2023

borut-t Jun 30, 2023

tonikocjan Oct 20, 2023

borut-t Jan 12, 2024

tonikocjan Jan 12, 2024 •

edited

Loading

borut-t May 12, 2023

tonikocjan May 12, 2023

borut-t Jan 12, 2024

borut-t Jun 30, 2023

borut-t left a comment

borut-t Jan 12, 2024

borut-t Jan 12, 2024

borut-t commented Jan 26, 2024

tonikocjan commented Feb 9, 2024

borut-t commented Feb 9, 2024

borut-t left a comment

Package re-structure #280

Package re-structure #280

Conversation

tonikocjan commented May 8, 2023 • edited Loading

borut-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonikocjan Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borut-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borut-t commented Jan 26, 2024

tonikocjan commented Feb 9, 2024

borut-t commented Feb 9, 2024

borut-t left a comment

Choose a reason for hiding this comment

tonikocjan commented May 8, 2023 •

edited

Loading

tonikocjan Jan 12, 2024 •

edited

Loading