-
Notifications
You must be signed in to change notification settings - Fork 471
Add SPM support & migrate to Swift 6.2 #295
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
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.