-
Notifications
You must be signed in to change notification settings - Fork 234
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
Use lower camel case names for error types in Swift bindings #2351
base: main
Are you sure you want to change the base?
Conversation
Yes please! What we have on |
I'm mildly reluctant to cause this kind of breakage - IIUC, it will break code far away from the uniffi managed world. Eg, I'm generally fine breaking, say, the UDL or the Rust code, because that will be very close to UniFFI - the developer responsible for fixing the breakage is probably the same one who is updating UniFFI. But in this kind of change, it's possible the breakage will be in code written by someone who has never heard of UniFFI (right?)
Maybe, but it would have to be a mild preference - eg, this has been asked for very rarely in issues here and never by our internal iOS team. Note that I'm not veto-ing this - I'd love to hear from others. |
Maybe it could be enabled through some configuration option, like the ones in https://github.com/mozilla/uniffi-rs/blob/main/docs/manual/src/swift/configuration.md ? |
Yes, if the agreement is that we don't want the breaking change, that's exactly what we would need to do, which wouldn't really complicate things too badly. |
c8c394c
to
3afb3c7
Compare
I have added a new configuration. By default ( I'd love to add tests to cover setting the new configuration to true, which would require the tests to override uniffi.toml configuration. But I didn't find a way to do that from the |
I think you could clone the test and supply a different uniffi.toml for one of them? |
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 looking great, thanks! Would it be possible to trim it down a little?
I'm sorry I said "clone", but we don't really need to copy all the stuff that's not related to this rename - so most of the functions can go, and the .swift can ideally do the minimum to ensure the variants are named correct - ideally just a few lines long. A few comments could explain how the name differs from when the .toml config value isn't set.
/// Like enum_variant_swift_quoted, but a class name. | ||
pub fn error_variant_swift_quoted(nm: &str) -> Result<String, rinja::Error> { | ||
Ok(quote_general_keyword(oracle().class_name(nm))) | ||
/// Get the Swift rendering of an individual enum variant for an Error type. |
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.
probably worth commenting here that the default name isn't actually preferred - ie, explaining why this exists?
This PR updates the Swift bindings code to use
lowerCamelCase
for error enum cases, to follow Swift's coding convention (example).I'm not sure it's worth adding a new Swift-specific config to
uniffi.toml
here. The PR introduces breaking changes, but they are easy to fix. I'd say most Swift developers would prefer the new generated code.Here is a before and after difference:
diff