-
Notifications
You must be signed in to change notification settings - Fork 571
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] Expose a few helper methods from SwiftGenerator #3115
Conversation
@@ -6,7 +6,7 @@ public struct SwiftEdgeCases { | |||
|
|||
@ProtoDefaulted | |||
public var `return`: String? | |||
public var error: SwiftEdgeCases.Error? | |||
public var error: SwiftEdgeCases.Error_? |
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.
@lickel the fact that I consolidated this under a single helper method it now affected enums just like it did messages, does it look good to you as a change?
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 also looks like enums called Type
are also emitted Type_
so this looks more aligned now
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
50a922e
to
307befc
Compare
public final fun getSchema ()Lcom/squareup/wire/schema/Schema; | ||
} | ||
|
||
public final class com/squareup/wire/swift/SwiftGenerator$Companion { | ||
public final fun builtInType (Lcom/squareup/wire/schema/ProtoType;)Z | ||
public final fun get (Lcom/squareup/wire/schema/Schema;Ljava/util/Map;)Lcom/squareup/wire/swift/SwiftGenerator; | ||
public static synthetic fun get$default (Lcom/squareup/wire/swift/SwiftGenerator$Companion;Lcom/squareup/wire/schema/Schema;Ljava/util/Map;ILjava/lang/Object;)Lcom/squareup/wire/swift/SwiftGenerator; | ||
public final fun getBUILT_IN_TYPES ()Ljava/util/Map; |
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 am structurally OK with this, but you should have someone like @oldergod sign off on this change since it's an expansion of the public API.
Also: I suspect there's "better" ways to expose the constants, so it's not getBUILT_IN_TYPES()
in Java-land
- Sign off from a maintainer
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 spoke to @oldergod offline about this to lead up to this PR but yes will wait for the approval here.
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.
@oldergod can we specify the name to expose for this tool so its not so ugly getBUILT_IN_TYPES
?
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.
You can add @JvmStatic
to the variable.
But instead of exposing the list of built-in types, could we just expose a method like fun builtInType(protoType: ProtoType): Boolean = protoType in BUILT_IN_TYPES.keys
?
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.
ha that is already exposed, ok will update to remove BUILT_IN_TYPES and SWIFT_COMMON_TYPES
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.
done
307befc
to
f37d049
Compare
These can be useful for custom schema handlers related to Swift.