-
Notifications
You must be signed in to change notification settings - Fork 62
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
Support failable initializers #276
Conversation
794363a
to
bc978aa
Compare
fn some_function(arg: u8) {} | ||
|
||
struct SomeType; | ||
|
||
impl SomeType { | ||
#[allow(unused)] | ||
fn some_method(&self, foo: u8) {} |
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.
Added #[allow(unused)]
to fix the following warnings.
error: Argument "arg_typo" was not found in "fn some_function(..)"
--> tests/ui/args-into-argument-not-found.rs:7:42
|
7 | #[swift_bridge(args_into = (arg, arg_typo))]
| ^^^^^^^^
error: Argument "bar" was not found in "fn some_method(..)"
--> tests/ui/args-into-argument-not-found.rs:13:42
|
13 | #[swift_bridge(args_into = (foo, bar))]
| ^^^
warning: unused variable: `arg`
--> tests/ui/args-into-argument-not-found.rs:19:18
|
19 | fn some_function(arg: u8) {}
| ^^^ help: if this is intentional, prefix it with an underscore: `_arg`
|
= note: `#[warn(unused_variables)]` on by default
warning: unused variable: `foo`
--> tests/ui/args-into-argument-not-found.rs:25:27
|
25 | fn some_method(&self, foo: u8) {}
| ^^^ help: if this is intentional, prefix it with an underscore: `_foo`
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.
Instead of #[allow(unused)]
let's do _arg
and _foo
. Less noisy.
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.
Great work. Left some minor feedback. Thanks for implementing this. Great to see you back.
|
||
func testFailableInitializer() { | ||
XCTAssertEqual(FailableInitType(false), nil) | ||
XCTAssertNotEqual(FailableInitType(true), nil) |
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.
Let's make sure that we can use the type. This gives us more confidence that we didn't, say, bridge Some(bool)
instead of Some(FailableInitType)
.
Maybe
let some = FailableInitType(true)
XCTAssertEqual(some!.count(), 123);
@@ -211,5 +211,10 @@ class OptionTests: XCTestCase { | |||
XCTAssertEqual(reflectedSome!.field, 123) | |||
XCTAssertNil(reflectedNone) | |||
} | |||
|
|||
func testFailableInitializer() { |
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.
From now on all tests should say what we're verifying.
/// Verify that ...
This helps:
- new maintainers since it's easier to read English than code.
- forces the author to be clear about what behavior they're trying to test. Needing to explain ourselves makes us think critically about what we are doing and if we're covering enough cases.
- the reviewer noticing missing tests. For example, if the reviewer sees that what the test says that it is testing doesn't seem to match the code inside, they can comment on that. This tends to increase the quality of a test suite.
For simple stuff like this a sentence is fine.
/// Verify that we generated a Swift class with a failable init method. | ||
#[test] | ||
fn class_with_failable_init() { |
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.
It's test this in the codegen_tests
module instead of here.
I think we should put all new codegen tests there going forward. This way there is a single place to find all codegen tests.
- more discoverable
- less confusion around where to put a test. there's always one place to put a codegen test instead of not knowing if it should go in
generate_swift.rs
ormod codegen_tests::*
} | ||
extension Foo { | ||
public convenience init?() { | ||
guard let val = __swift_bridge__$Foo$new() else { return nil }; self.init(ptr: val) |
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.
nice
My feedback was minor, so feel free to When you Alternatively you can rebase into a single commit, force push that, and then merge that. But |
Hello, hello. Still planning to land this? |
Sorry. I'll address your review this weekend if possible. |
b4dade6
to
0125875
Compare
0125875
to
55ab89c
Compare
@@ -456,3 +456,74 @@ typedef struct MyType MyType; | |||
.test(); | |||
} | |||
} | |||
|
|||
/// Verify that we generated a Swift class with a failable init method. | |||
mod extern_rust_class_with_failable_init { |
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.
@chinedufn
I guess it's suitable to add the test here.
Related: #276 (comment)
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.
Looks good
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.
Looks good just one small request then you can merge this.
Please remember to squash and merge and use your PR title for the commit title and PR body for the commit body.
@@ -582,6 +582,7 @@ | |||
COMBINE_HIDPI_IMAGES = YES; | |||
CURRENT_PROJECT_VERSION = 1; | |||
DEVELOPMENT_ASSET_PATHS = "\"SwiftRustIntegrationTestRunner/Preview Content\""; | |||
DEVELOPMENT_TEAM = ""; |
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.
Do we need these two DEVELOPMENT_TEAM
lines? If not lets revert them.
Related to #235.
This PR supports failable initializers.
Something like:
See: Swift Documentation - Failable Initializers