-
Notifications
You must be signed in to change notification settings - Fork 7
Change Rooms get/release parameter labels
#420
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
Conversation
WalkthroughRenamed the Rooms API parameter label from name: to named: across public protocols, implementations, mocks, example app, and tests. Updated associated release methods to release(named:). Adjusted documentation to include a platform flag in the example build command. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (7)
🧰 Additional context used📓 Path-based instructions (3)**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
Tests/**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
Sources/**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (6)Tests/AblyChatTests/ChatClientTests.swift (3)
Example/AblyChatExample/ContentView.swift (3)
Example/AblyChatExample/Mocks/MockClients.swift (2)
Sources/AblyChat/Rooms.swift (3)
Tests/AblyChatTests/DefaultRoomsTests.swift (3)
Tests/AblyChatTests/IntegrationTests.swift (3)
🪛 LanguageToolCLAUDE.md[grammar] ~46-~46: There might be a mistake here. (QB_NEW_EN) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I say we go the full hog and align it with what Apple does with UIImage here... the user would still call |
isn't that what this PR is proposing? |
Switch to: - rooms.get(named: "basketball") instead of rooms.get(name: "basketball") - rooms.release(named: "basketball") instead of rooms.release(name: "basketball") In line with the fluency changes of c32a2fa, in particular the "Prefer method and function names that make use sites form grammatical English phrases" of [1]. withName: or forName: were also an option. I couldn't get an LLM to give me any useful answers; it kept telling me that everything I suggested was marvellous and then doubting itself on everything when pressed. There's UIImage(named name: String) in Apple's SDKs, for comparison. [1] https://www.swift.org/documentation/api-design-guidelines/
It was trying to run BuildTool, getting an error about no platform specified, and then just trying to run xcodebuild instead.
a57c6fa to
e5c84f0
Compare
action(name:) to action(named:)get/release parameter labels
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.
I agree with the name.
rooms.get(named: "basketball")instead ofrooms.get(name: "basketball")rooms.release(named: "basketball")instead ofrooms.release(name: "basketball")in line with the fluency changes of #379, in particular the "Prefer method and function names that make use sites form grammatical English phrases" of https://www.swift.org/documentation/api-design-guidelines/
Thoughts?
withName:orforName:are also an option. I couldn't get an LLM to give me any useful answers; it kept telling me that everything I suggested was marvellous. There'sUIImage(named name: String)in Apple's SDKs, for comparison.Summary by CodeRabbit