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

Optimise the Swift version of Vexilla Client #42

Merged
merged 8 commits into from Mar 17, 2024

Conversation

Joannis
Copy link
Contributor

@Joannis Joannis commented Mar 14, 2024

  1. Move SwiftPM's Package.swift into the repo's root. This is required for SwiftPM projects to depend on this package.
  2. Add a VSCode recommendation for the SSWG plugin
  3. Remove inline API call implementations, which leans on URLSession. API calls are extracted into a new test-wide implementation.
  4. Adopt XCTAssertGreaterThan-family of functions for comparing values
  5. Support unicode characters in String hashing algorithm, also optimises the algorithm slightly - reducing memory copies.
  6. Add concrete error types, replacing some String errors.
  7. Remove safeGet and all code that leveraged this.
  • Moved JSON parsing over to JSONDecoder
  • Accessing dictionaries now uses the subscript
  • This allows errors to be added for specific scenarios
  • Reduce cyclomatic complexity through guard statements

cmgriffing: editing this description to hopefully kick of the github action (x3)

Since safeGet is not used, we can add custom error messages that clarify the specific issue at hand. Also by leveraging `#function` you can trace back the origin of the error. Note: String errors should still be migrated over to concrete types.
This commit also adopts `guard let` syntax to keep cyclomatic complexity low.
Copy link

netlify bot commented Mar 14, 2024

👷 Deploy request for forms-vexilla pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 3381137

Copy link

netlify bot commented Mar 14, 2024

👷 Deploy request for auth-vexilla pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 3381137

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2024

CLA assistant check
All committers have signed the CLA.

@cmgriffing
Copy link
Contributor

The "PR Test / test-swift" job seems to be failing.

Here is the first error that gets repeated a bunch.

/__w/vexilla/vexilla/clients/swift/Sources/VexillaClient/Types.swift:85:33: error: expected ',' separator
  public init(from decoder: any Decoder) throws {

The second error that repeats a lot with slight variations is:

/__w/vexilla/vexilla/clients/swift/Sources/VexillaClient/Types.swift:253:62: error: cannot infer contextual base in reference to member 'value'
      let value = try container.decode(String.self, forKey: .value)

@Joannis
Copy link
Contributor Author

Joannis commented Mar 15, 2024

Right, Swift toolchain difference. I forgot about some of these language features having been added more recently.

@cmgriffing
Copy link
Contributor

Not sure why the tests didn't kick off when you updated it. That is a bug on my end in the workflow. However, after an edit of the description of the PR, I was able to kick it off again and the tests now pass inside of the container.

@@ -226,7 +226,6 @@ jobs:
steps:
- uses: actions/checkout@v3
- run: |
cd ./clients/swift
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (blocking): I would prefer not to move the Package.swift file to the top level. With plans for 14 SDKs, the root level of the project could become unwieldy if all clients SDKs did this.

Does having the Package.swift in a nested folder break something for SwiftPM?

@cmgriffing cmgriffing merged commit 01db167 into Vexilla:main Mar 17, 2024
7 checks passed
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.

None yet

3 participants