Add SPM support & migrate to Swift 6.2#295
Add SPM support & migrate to Swift 6.2#295sereivoanyong wants to merge 1 commit intobannzai:masterfrom
Conversation
Summary of ChangesHello @sereivoanyong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces Swift Package Manager support and migrates the project to Swift 6.2, aligning with modern Swift development practices. A significant focus has been placed on enhancing concurrency safety by applying the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request does a great job of adding Swift Package Manager support and migrating the codebase to Swift 6.2. The changes to adopt Swift 6's concurrency features, like using @MainActor for UI-related classes and adding Sendable conformance, are well-executed and necessary for modern Swift development. The updates from var to let for static constants and class to AnyObject for protocols are also good practice.
My main feedback is on the new Package.swift file. I've suggested adding platform specifications and including the existing test suite to make the SPM package more robust and easier for other developers to use.
| // swift-tools-version: 6.2 | ||
|
|
||
| import PackageDescription | ||
|
|
||
| let package = Package( | ||
| name: "SpreadsheetView", | ||
| products: [ | ||
| .library(name: "SpreadsheetView", targets: ["SpreadsheetView"]), | ||
| ], | ||
| targets: [ | ||
| .target(name: "SpreadsheetView", path: "Framework/Sources"), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
This is a great start for adding Swift Package Manager support! To make the package manifest more complete and useful for developers, I have a couple of suggestions:
-
Specify Supported Platforms: Since this library uses
UIKit, it's specific to Apple platforms like iOS. It's a good practice to explicitly declare the supported platforms and minimum deployment versions. Based on the usage of APIs likesafeAreaInsetsDidChange, it seems iOS 11 would be a safe minimum target. -
Include a Test Target: The repository contains tests under
Framework/Tests. These should be included in thePackage.swiftfile so they can be run easily using SPM.
I've included a code suggestion that incorporates these changes.
| // swift-tools-version: 6.2 | |
| import PackageDescription | |
| let package = Package( | |
| name: "SpreadsheetView", | |
| products: [ | |
| .library(name: "SpreadsheetView", targets: ["SpreadsheetView"]), | |
| ], | |
| targets: [ | |
| .target(name: "SpreadsheetView", path: "Framework/Sources"), | |
| ] | |
| ) | |
| // swift-tools-version: 6.2 | |
| import PackageDescription | |
| let package = Package( | |
| name: "SpreadsheetView", | |
| platforms: [ | |
| .iOS(.v11) | |
| ], | |
| products: [ | |
| .library(name: "SpreadsheetView", targets: ["SpreadsheetView"]), | |
| ], | |
| targets: [ | |
| .target(name: "SpreadsheetView", path: "Framework/Sources"), | |
| .testTarget( | |
| name: "SpreadsheetViewTests", | |
| dependencies: ["SpreadsheetView"], | |
| path: "Framework/Tests") | |
| ] | |
| ) |
No description provided.