Skip to content
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

Swift: Wire initializer refactor #2561

Merged
merged 7 commits into from
Aug 17, 2023
Merged

Swift: Wire initializer refactor #2561

merged 7 commits into from
Aug 17, 2023

Conversation

lickel
Copy link
Collaborator

@lickel lickel commented Aug 16, 2023

This removes the member-wise initializer by default.

There's two reasons for this:

  • This is believed to cause code bloat
  • This will have unexpected behaviors as we better handle proto extensions

Instead of a member wise initializer, this exposes a configuration closure for any optional parameters.
There's still guaranteed type safety as all required parameters must have initialized values.

Note

Memberwise initializers should now be considered deprecated and will be removed in the near future (for example, November 2023)

TODO:

  • Add deprecation annotations
  • Remove configure when it's irrelevant

@oldergod
Copy link
Member

./gradlew spotlessApply

public init(
seconds: Int64,
nanos: Int32,
configure: (inout Self) -> Void = { _ in }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in having the configure block if all of the fields are required? (Or if there are no fields, like the OneOfOptions below?)

#if WIRE_INCLUDE_MEMBERWISE_INITIALIZER
extension Dinosaur {

@_disfavoredOverload
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe I realized I would need it immediately for a bunch of types 🙂

@@ -255,7 +259,7 @@ class SwiftGenerator private constructor(
fileMembers: MutableList<FileMemberSpec>,
): List<TypeSpec> {
val structType = type.typeName
val oneOfEnumNames = type.oneOfs.associateWith { structType.nestedType(it.name.capitalize(US)) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a deprecation warning that Kotlin keeps yelling about.

return
}

val memberwiseExtension = ExtensionSpec.builder(structType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment noting we want to remove this, but it's left in temporarily for compatibility.

@@ -180,13 +189,26 @@ public struct Form {

public var unknownFields: Foundation.Data = .init()

public init() {
public init(configure: (inout Self) -> Void = { _ in }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should remove the configure if there are no params.

@lickel lickel merged commit dae04ab into master Aug 17, 2023
23 of 24 checks passed
@lickel lickel deleted the alickel/wire-init branch August 17, 2023 03:54
github-merge-queue bot referenced this pull request in slackhq/foundry Aug 21, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[com.squareup.wire:wire-gradle-plugin](https://togithub.com/square/wire)
| dependencies | patch | `4.8.0` -> `4.8.1` |

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>square/wire (com.squareup.wire:wire-gradle-plugin)</summary>

###
[`v4.8.1`](https://togithub.com/square/wire/blob/HEAD/CHANGELOG.md#Version-481)

[Compare Source](https://togithub.com/square/wire/compare/4.8.0...4.8.1)

*2023-08-17*

- New: Swift messages now have the form `init(REQUIRED FIELDS, (inout
Storage) -> Void)`
- New: Swift, the member-wise initializer has been removed by default.
It can be re-enabled by defining `WIRE_INCLUDE_MEMBERWISE_INITIALIZER`;
however, it will be removed in November 2024. See
[https://github.com/square/wire/pull/2561](https://togithub.com/square/wire/pull/2561)
for details
- Fix: Correctly define sources folders vs. resources folders for Wire
generated code.
-   Fix: Generated `.proto` are correctly added to the built artifact.
-   New: All options of KotlinTarget available on CLI.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41Mi4yIiwidXBkYXRlZEluVmVyIjoiMzYuNTIuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
dnkoutso added a commit that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants