Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the project's tooling and linting infrastructure by switching from Make to Just, integrating pre-commit hooks, adding typos checking, and enabling SwiftLint. The changes fix various typos throughout the codebase and add SwiftLint compliance markers to suppress warnings where appropriate.
Changes:
- Replaced Makefile with justfile and switched build commands to use Just
- Added pre-commit configuration with typos checking and swiftformat linting
- Integrated SwiftLint with configuration file and compliance fixes across the codebase
- Fixed multiple typos in comments, documentation, and error messages
- Added workspace structure for the scripts package
- Added unsafeFlags to Package.swift to suppress C compiler warnings
Reviewed changes
Copilot reviewed 80 out of 83 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Package.swift | Inlined cSettings and added unsafeFlags to suppress C warnings |
| Makefile | Deleted - replaced by justfile |
| justfile | New command runner with various development tasks |
| scripts/bootstrap | Enhanced to install just, pre-commit, and typos |
| .pre-commit-config.yaml | New pre-commit configuration for typos and swiftformat |
| .swiftlint.yml | New SwiftLint configuration |
| .swiftformat | Updated to exclude Tests directory |
| _typos.toml | Configuration for typos spell checker |
| scripts/update-libsecp/* | Fixed typos and added swiftlint compliance markers |
| Sources/K1/**/*.swift | Added swiftlint disable/enable markers for various rules |
| Tests/K1Tests/**/*.swift | Fixed typos, improved variable naming, added swiftlint markers |
| README.md | Fixed typos and updated development instructions |
| .github/workflows/swift.yml | Added typos check step and updated to use just instead of make |
| LICENSE.txt | Updated copyright year to 2022-2026 |
| K1.xcworkspace/* | Added workspace structure for scripts package |
Files not reviewed (1)
- K1.xcworkspace/contents.xcworkspacedata: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| first.append(Data("world".utf8)) | ||
| second.append(Data("wrold".utf8)) | ||
| second.append(Data("sweden".utf8)) |
There was a problem hiding this comment.
The test data change from "wrold" to "sweden" appears to be intentional to fix a typo in the test. However, this changes the semantics of the test - "wrold" was likely an intentional misspelling to ensure inequality, while "sweden" is a completely different word. Consider whether "world" (the correct spelling) or another misspelling like "wrold" would be more appropriate to maintain the test's original intent of checking inequality after appending different data.
| let privateKey = try K1.KeyAgreement.PrivateKey(rawRepresentation: privateBytes) | ||
|
|
||
| /// ANS1 X9.63 serialization of shared secret, returning a `CryptoKit.SharedSecret` | ||
| // ANS1 X9.63 serialization of shared secret, returning a `CryptoKit.SharedSecret` |
There was a problem hiding this comment.
The comment type was changed from a triple-slash comment (///) to a double-slash comment (//). Triple-slash comments are typically used for documentation in Swift. If this change was intentional to indicate this is not documentation but rather a regular comment, that's fine. However, verify this is the intended behavior, as it will remove this from generated documentation.
| /// `R || S || V` - the format `libsecp256k1` v0.3.0 uses as representation | ||
| /// This is the default value of this library. | ||
| case rsv | ||
|
|
||
| /// We use `R || S || V` as default values since `libsecp256k1` v0.3.0 uses it as its internal representation. | ||
| /// We use `R || S || V` as default values since `libsecp256k1` v0.3.0 uses it as its representation. |
There was a problem hiding this comment.
The word "internal" was removed from the comment changing "R || S || V - the format libsecp256k1 v0.3.0 uses as internal representation" to "R || S || V - the format libsecp256k1 v0.3.0 uses as representation". While this simplifies the comment, "internal representation" is more technically accurate as it specifically refers to how the library stores this data internally. Consider keeping "internal" for clarity.
| // We expect a single child. | ||
| guard case .constructed(let nodes) = node.content else { | ||
| // This error is an internal parser error: the tag above is always constructed. | ||
| // This error is an parser error: the tag above is always constructed. |
There was a problem hiding this comment.
In line 204, the comment was changed from "This error is an internal parser error" to "This error is an parser error". This introduces a grammatical error - it should be "a parser error" not "an parser error". Consider reverting this change or fixing it to "a parser error".
|
|
||
| // Suppress build warnings | ||
| .unsafeFlags( | ||
| ["-Wno-shorten-64-to-32"] | ||
| ), |
There was a problem hiding this comment.
The use of unsafeFlags in Package.swift to suppress C warnings ("-Wno-shorten-64-to-32") is noted. While this resolves the immediate warning issue mentioned in the PR description, be aware that unsafeFlags can cause issues with package caching and may be rejected by some package consumers. Consider whether these warnings could be addressed at the source level in the C code instead, or document why these warnings are acceptable to suppress.
| // Suppress build warnings | |
| .unsafeFlags( | |
| ["-Wno-shorten-64-to-32"] | |
| ), |
Switch to just from makefile
Remove legacy pre-commit hooks, replaced with pre-commit binary
Fix typos and add typos pre-commit
Add workspace (for the script package.swift added recently)
Add unsafeFlags to suppress C warnings
Remove scripts/gyb.pyc from git and gitignore it
Make swiftlint successfully lint project (but continue to use swiftformat)