-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Typos, test coverage, and documentation #883
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: main
Are you sure you want to change the base?
Typos, test coverage, and documentation #883
Conversation
…lor+Peer - Add comprehensive documentation to String+DJB2 explaining the hash algorithm and its use case - Add enhanced documentation to String+Nickname with usage examples - Add StringUtilsTests with 24 test cases covering: * DJB2 hash consistency, case sensitivity, unicode handling * Nickname suffix parsing edge cases and validation - Add ColorPeerTests with 20 test cases covering: * Color consistency and caching behavior * Dark/light mode color differentiation * Orange hue avoidance for mention highlighting * Real-world scenarios with Nostr pubkeys and edge cases
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @Test func peerColor_sameSeedProducesSameColor() { | ||
| let color1 = Color(peerSeed: "alice", isDark: false) | ||
| let color2 = Color(peerSeed: "alice", isDark: false) | ||
|
|
||
| // Since the cache is static, both should produce the same color | ||
| // We can't directly compare Color objects, but we can verify they produce consistent hashes | ||
| #expect(color1.description == color2.description, "Same seed should produce same color") | ||
| } | ||
|
|
||
| @Test func peerColor_differentSeedsProduceDifferentColors() { | ||
| let color1 = Color(peerSeed: "alice", isDark: false) | ||
| let color2 = Color(peerSeed: "bob", isDark: false) |
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.
Add missing Color(peerSeed:isDark:) initializer
The new tests call Color(peerSeed:isDark:) but the production module does not define such an initializer anywhere in the repository. Because SwiftUI’s Color has no initializer with this signature, the test suite will not even compile once this file is included in the build. Either the initializer should be implemented in the app target or the tests need to use an existing API.
Useful? React with 👍 / 👎.
| func splitSuffix() -> (String, String) { | ||
| let name = self.replacingOccurrences(of: "@", with: "") | ||
| guard name.count >= 5 else { return (name, "") } | ||
| let suffix = String(name.suffix(5)) | ||
| if suffix.first == "#", suffix.dropFirst().allSatisfy({ c in | ||
| ("0"..."9").contains(String(c)) || ("a"..."f").contains(String(c)) || ("A"..."F").contains(String(c)) | ||
| }) { | ||
| let base = String(name.dropLast(5)) | ||
| return (base, suffix) |
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.
splitSuffix accepts nicknames with double '#' suffix
The new splitSuffix() treats any string whose last five characters look like # plus four hex digits as a valid suffix. This means names such as "test##1234" are parsed into base "test#" and suffix "#1234", even though the extra # indicates the string does not follow the expected <name>#<hex> form. The accompanying tests expect such inputs to be rejected, so the implementation should also verify that the character before the suffix is not '#' (or otherwise validate that only one delimiter is present) before splitting.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.