-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[pigeon][swift] Removes FlutterError in favor of PigeonError #6611
Merged
+396
−278
Merged
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
c510bd6
[pigeon] Do not use FlutterError when generating Swift code
bc-lee d8968d7
[pigeon] Add option to whether emit PigeonError class in Swift code
bc-lee c2beea1
[pigeon] Simplify localizedDescription of PigeonError in Swift
bc-lee 6a8a94c
[pigeon] Always make sure CoreTests.gen.swift has PigeonError class
bc-lee b874f09
[pigeon] Make PigeonError class final and add FlutterError initializer
bc-lee df102e3
[pigeon] Rename swiftEmitErrorClass to emitPigeonErrorClass
bc-lee b30a0cc
[pigeon] Make wrapError in swift also handle PigeonError
bc-lee 30ac050
[pigeon] Emit Error class for Swift with different names per file
bc-lee 92c97af
[pigeon] Restore docregion for swift-class
bc-lee 86d9168
[pigeon] Add CHANGELOG.md entry
bc-lee 20ac87a
[pigeon] Apply format fix
bc-lee 800ba60
[pigeon] Generate Swift error on CoreTests.gen.swift
bc-lee 8fa6b75
[pigeon] Fix unittests for swift generator
bc-lee 4bd3dbc
[pigeon] Run update-excerpts
bc-lee fae49ea
[pigeon] Bump version correctly
bc-lee b386a96
Merge commit 'd670b2c38c8db0a773aa703e7d3f15682e05ad7f' into feature/…
bc-lee 78c3bb6
Remove conversion from FlutterError to PigeonError in Swift
bc-lee f13a06a
Update the comment for the PigeonError class in Swift
bc-lee 63df915
Update the documentation for using PigeonError in Swift
bc-lee 428736d
Replace several usage of Indent.nest with Indent.writeScoped
bc-lee 88cd27d
Update Changelog and README of pigeon package
bc-lee a78cae7
Addressing review comments
bc-lee 4ed7852
Relocate PigeonError class in Swift files
bc-lee a8ab4f8
Merge branch 'main' into feature/pigeon-swift-error
tarrinneal 877015d
Merge branch 'main' into feature/pigeon-swift-error
bc-lee File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,16 +19,21 @@ class SwiftOptions { | |
/// Creates a [SwiftOptions] object | ||
const SwiftOptions({ | ||
this.copyrightHeader, | ||
this.errorClassName, | ||
}); | ||
|
||
/// A copyright header that will get prepended to generated code. | ||
final Iterable<String>? copyrightHeader; | ||
|
||
/// The name of the error class used for passing custom error parameters. | ||
final String? errorClassName; | ||
|
||
/// Creates a [SwiftOptions] from a Map representation where: | ||
/// `x = SwiftOptions.fromList(x.toMap())`. | ||
static SwiftOptions fromList(Map<String, Object> map) { | ||
return SwiftOptions( | ||
copyrightHeader: map['copyrightHeader'] as Iterable<String>?, | ||
errorClassName: map['errorClassName'] as String?, | ||
); | ||
} | ||
|
||
|
@@ -37,6 +42,7 @@ class SwiftOptions { | |
Map<String, Object> toMap() { | ||
final Map<String, Object> result = <String, Object>{ | ||
if (copyrightHeader != null) 'copyrightHeader': copyrightHeader!, | ||
if (errorClassName != null) 'errorClassName': errorClassName!, | ||
}; | ||
return result; | ||
} | ||
|
@@ -316,7 +322,7 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> { | |
name: func.name, | ||
parameters: func.parameters, | ||
returnType: func.returnType, | ||
errorTypeName: 'FlutterError', | ||
errorTypeName: _getErrorClassName(generatorOptions), | ||
isAsynchronous: true, | ||
swiftFunction: func.swiftFunction, | ||
getParameterName: _getSafeArgumentName, | ||
|
@@ -350,6 +356,7 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> { | |
indent, func.documentationComments, _docCommentSpec); | ||
_writeFlutterMethod( | ||
indent, | ||
generatorOptions: generatorOptions, | ||
name: func.name, | ||
channelName: makeChannelName(api, func, dartPackageName), | ||
parameters: func.parameters, | ||
|
@@ -614,7 +621,7 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> { | |
}); | ||
} | ||
|
||
void _writeWrapError(Indent indent) { | ||
void _writeWrapError(SwiftOptions generatorOptions, Indent indent) { | ||
indent.newln(); | ||
indent.write('private func wrapError(_ error: Any) -> [Any?] '); | ||
indent.addScoped('{', '}', () { | ||
tarrinneal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -627,6 +634,16 @@ class SwiftGenerator extends StructuredGenerator<SwiftOptions> { | |
indent.writeln('flutterError.details,'); | ||
}); | ||
}); | ||
indent.write( | ||
'if let pigeonError = error as? ${_getErrorClassName(generatorOptions)} '); | ||
indent.addScoped('{', '}', () { | ||
indent.write('return '); | ||
indent.addScoped('[', ']', () { | ||
indent.writeln('pigeonError.code,'); | ||
indent.writeln('pigeonError.message,'); | ||
indent.writeln('pigeonError.details,'); | ||
}); | ||
}); | ||
tarrinneal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
indent.write('return '); | ||
indent.addScoped('[', ']', () { | ||
indent.writeln(r'"\(error)",'); | ||
|
@@ -645,13 +662,14 @@ private func nilOrValue<T>(_ value: Any?) -> T? { | |
}'''); | ||
} | ||
|
||
void _writeCreateConnectionError(Indent indent) { | ||
void _writeCreateConnectionError( | ||
SwiftOptions generatorOptions, Indent indent) { | ||
indent.newln(); | ||
indent.writeScoped( | ||
'private func createConnectionError(withChannelName channelName: String) -> FlutterError {', | ||
'private func createConnectionError(withChannelName channelName: String) -> ${_getErrorClassName(generatorOptions)} {', | ||
'}', () { | ||
indent.writeln( | ||
'return FlutterError(code: "channel-error", message: "Unable to establish connection on channel: \'\\(channelName)\'.", details: "")'); | ||
'return ${_getErrorClassName(generatorOptions)}(code: "channel-error", message: "Unable to establish connection on channel: \'\\(channelName)\'.", details: "")'); | ||
}); | ||
} | ||
|
||
|
@@ -671,17 +689,20 @@ private func nilOrValue<T>(_ value: Any?) -> T? { | |
|
||
if (hasHostApi) { | ||
_writeWrapResult(indent); | ||
_writeWrapError(indent); | ||
_writeWrapError(generatorOptions, indent); | ||
} | ||
if (hasFlutterApi) { | ||
_writeCreateConnectionError(indent); | ||
_writeCreateConnectionError(generatorOptions, indent); | ||
} | ||
|
||
_writePigeonError(generatorOptions, indent); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One last nit and we can land it, can you move the pigeon error above the other methods here? above `if (hasHostApi) {' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
_writeIsNullish(indent); | ||
_writeNilOrValue(indent); | ||
} | ||
|
||
void _writeFlutterMethod( | ||
Indent indent, { | ||
required SwiftOptions generatorOptions, | ||
required String name, | ||
required String channelName, | ||
required List<Parameter> parameters, | ||
|
@@ -693,7 +714,7 @@ private func nilOrValue<T>(_ value: Any?) -> T? { | |
name: name, | ||
parameters: parameters, | ||
returnType: returnType, | ||
errorTypeName: 'FlutterError', | ||
errorTypeName: _getErrorClassName(generatorOptions), | ||
isAsynchronous: true, | ||
swiftFunction: swiftFunction, | ||
getParameterName: _getSafeArgumentName, | ||
|
@@ -735,12 +756,12 @@ private func nilOrValue<T>(_ value: Any?) -> T? { | |
indent.writeln('let message: String? = nilOrValue(listResponse[1])'); | ||
indent.writeln('let details: String? = nilOrValue(listResponse[2])'); | ||
indent.writeln( | ||
'completion(.failure(FlutterError(code: code, message: message, details: details)))'); | ||
'completion(.failure(${_getErrorClassName(generatorOptions)}(code: code, message: message, details: details)))'); | ||
}, addTrailingNewline: false); | ||
if (!returnType.isNullable && !returnType.isVoid) { | ||
indent.addScoped('else if listResponse[0] == nil {', '} ', () { | ||
indent.writeln( | ||
'completion(.failure(FlutterError(code: "null-error", message: "Flutter api returned null value for non-null return value.", details: "")))'); | ||
'completion(.failure(${_getErrorClassName(generatorOptions)}(code: "null-error", message: "Flutter api returned null value for non-null return value.", details: "")))'); | ||
}, addTrailingNewline: false); | ||
} | ||
indent.addScoped('else {', '}', () { | ||
|
@@ -870,11 +891,41 @@ private func nilOrValue<T>(_ value: Any?) -> T? { | |
indent.writeln('$varChannelName.setMessageHandler(nil)'); | ||
}); | ||
} | ||
|
||
void _writePigeonError(SwiftOptions generatorOptions, Indent indent) { | ||
indent.newln(); | ||
indent.writeln( | ||
'/// Error class for passing custom error details to Dart side.'); | ||
indent.writeScoped( | ||
'final class ${_getErrorClassName(generatorOptions)}: Error {', '}', | ||
() { | ||
indent.writeln('let code: String'); | ||
indent.writeln('let message: String?'); | ||
indent.writeln('let details: Any?'); | ||
indent.newln(); | ||
indent.writeScoped( | ||
'init(code: String, message: String?, details: Any?) {', '}', () { | ||
indent.writeln('self.code = code'); | ||
indent.writeln('self.message = message'); | ||
indent.writeln('self.details = details'); | ||
}); | ||
indent.newln(); | ||
indent.writeScoped('var localizedDescription: String {', '}', () { | ||
indent.writeScoped('return', '', () { | ||
indent.writeln( | ||
'"${_getErrorClassName(generatorOptions)}(code: \\(code), message: \\(message ?? "<nil>"), details: \\(details ?? "<nil>")"'); | ||
}, addTrailingNewline: false); | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
/// Calculates the name of the codec that will be generated for [api]. | ||
String _getCodecName(Api api) => '${api.name}Codec'; | ||
|
||
String _getErrorClassName(SwiftOptions generatorOptions) => | ||
generatorOptions.errorClassName ?? 'PigeonError'; | ||
|
||
String _getArgumentName(int count, NamedType argument) => | ||
argument.name.isEmpty ? 'arg$count' : argument.name; | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
is it open for developers to use a custom class name? why don't we just use
PigeonError
?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.
Generally, I believe calling pigeon once for each package (plugin, app, etc.) should suffice. In these scenarios, there's no need to concern ourselves with the generated class name. However, in this repo, pigeon is invoked multiple times for the same package, which results in multiple classes with the same name, ultimately causing a compilation error.
Kotlin encounters a similar problem but has a solution for it. This Link provides a background of the issue as well as its resolution. I have therefore applied the same solution to the generation of Swift code.
I believe overriding the error class name as an advanced usage, so I prefer not to integrate it with the command line option, similar to the approach taken by Kotlin.