apple / swift-protobuf

Plugin and runtime library for using protobuf with Swift
Apache License 2.0
4.55k stars 443 forks source link

Split error enums based on operations #205

Closed thomasvl closed 7 years ago

thomasvl commented 7 years ago

I believe this has come up on some PRs, but never got captured.

Currently the source has one large enum for decoding and another for encoding. It likely would be cleaner if there were enums specific to JSON, Binary, and TextFormat, so it is easier for developers to ensure they handle all the relevant errors that they want for a given call.

tbkka commented 7 years ago

Any strong opinions about which of the following would be cleaner?

I'm leaning towards the first one...

allevato commented 7 years ago

JSONDecoder is essentially an internal type that users don't/shouldn't have to touch directly to use JSON serialization and is only public as an implementation detail, so I'd avoid nesting types that are user-visible in it for that reason. So I'm leaning toward JSONDecodingError, et al.

tbkka commented 7 years ago

A related style issue: There are a number of failure cases for specific WKTs: Timestamp, Duration, and FieldMask, for example, all have additional decoding constraints that can fail.

So if I have JSON {"time": "2017-02-malformed"}, is that a JSONDecodingError.malformedTimestamp or is that a Google_Protobuf_Timestamp.Error.malformed? I think I need to experiment a little to see how these Error enums work in practice...

tbkka commented 7 years ago

So if you catch let e to capture a thrown error and then simply print(e), all you get is the case name, and print(type(of: e)) just shows the immediate type name.

So attaching an Error enum to Timestamp for example isn't all that useful, since after you throw Timestamp.Error.malformed, the easiest information for the user to discover is malformed of type Error. This argues for a flatter approach with relatively verbose error names.

tbkka commented 7 years ago

PR #243 does the first part of this: It splits out the error enums for the decode side.

tbkka commented 7 years ago

PR #300 finishes this; the encoding errors are now also broken out by format.