-
Notifications
You must be signed in to change notification settings - Fork 113
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
Make TopLevelEncoder implementation overridable #182
Make TopLevelEncoder implementation overridable #182
Conversation
// MARK: - TopLevelEncoder | ||
|
||
open func encode<T>(_ value: T) throws -> Data where T: Encodable { | ||
try encode(value, withRootKey: nil, rootAttributes: nil, header: 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.
try encode(value, withRootKey: nil, rootAttributes: nil, header: nil) | |
return try encode(value, withRootKey: nil, rootAttributes: nil, header: 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.
I fixed in 84c33e7.
@@ -27,6 +27,12 @@ private struct Foo: Codable { | |||
var name: String | |||
} | |||
|
|||
final class CustomEncoder: XMLEncoder { | |||
override func encode<T>(_ value: T) throws -> Data where T : Encodable { | |||
try self.encode(value, withRootKey: "bar", rootAttributes: nil, header: 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.
try self.encode(value, withRootKey: "bar", rootAttributes: nil, header: nil) | |
return try self.encode(value, withRootKey: "bar", rootAttributes: nil, header: 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.
I fixed in 84c33e7.
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.
Thanks for the PR. I've added a few suggestions, as we should maintain backwards compatibility with Swift 4.2 syntax. Even though Combine is not available with Swift 4.2, it still seems like older compiler versions still attempt to parse and type check this code.
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
=======================================
Coverage 73.32% 73.32%
=======================================
Files 43 43
Lines 2332 2332
=======================================
Hits 1710 1710
Misses 622 622
Continue to review full report at Codecov.
|
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.
Thanks! I have to request a few more changes to make the SwiftFormat check pass on CI.
@@ -27,6 +27,12 @@ private struct Foo: Codable { | |||
var name: String | |||
} | |||
|
|||
final class CustomEncoder: XMLEncoder { | |||
override func encode<T>(_ value: T) throws -> Data where T : Encodable { |
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.
override func encode<T>(_ value: T) throws -> Data where T : Encodable { | |
override func encode<T>(_ value: T) throws -> Data where T: Encodable { |
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'm sorry I didn't check SwiftFormat. I fixed in 18282b5.
@@ -27,6 +27,12 @@ private struct Foo: Codable { | |||
var name: String | |||
} | |||
|
|||
final class CustomEncoder: XMLEncoder { | |||
override func encode<T>(_ value: T) throws -> Data where T : Encodable { | |||
return try self.encode(value, withRootKey: "bar", rootAttributes: nil, header: 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.
return try self.encode(value, withRootKey: "bar", rootAttributes: nil, header: nil) | |
return try encode(value, withRootKey: "bar", rootAttributes: nil, header: 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.
I fixed in 18282b5.
@MaxDesiatov It seems that danger-lint has failed in |
No worries, that seems like a SwiftLint bug 🙂 |
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 stuff, thank you 👍
`TopLevelEncoder` implementation was added by CoreOffice#175. However, its method cannot be overridden. If it can be, we can use custom root key, root attributes, or header even when we use Combine-style like this: ```swift final class MyEncoder: XMLEncoder { override func encode<T>(_ value: T) throws -> Data where T : Encodable { try self.encode(value, withRootKey: "foo", rootAttributes: nil, header: XMLHeader(version: 1.0, encoding: "UTF-8")) } } ```
TopLevelEncoder
implementation was added by #175. However, its method cannot be overridden. If it can be, we can use custom root key, root attributes, or header even when we use Combine-style like this:So, I proposed the solution.