Use string constants for region names #32
Conversation
Sources/S3Signer/Region.swift
Outdated
| /// name of the region, see RegionName | ||
| public let name: RegionName | ||
| /// name of the region, see Name | ||
| public let name: String |
There was a problem hiding this comment.
this introduces a breaking change that won't bring much benefit to most users, could you please keep the original enum (it was Codable so you can store info about easily in a db for example) and just add .custom(String) as a case? I thought that's what was suggested on discord?
|
Thanks for the comments! I indeed suggested that adding a custom case to the enum could solve the issue, but later I realised that this is not the best solution. Here’s why:
enums are typically used in situations where a variable can take one of several _fixed_ values. And swift made it easy to ‘switch’ on the enum in an _exhaustive_ way. However, regions are neither fixed (they can change from day to day depending on the vender) nor can be listed exhaustively (there are infinitely many possible regions). Hence I think enums would be a poor way to model them. (Another piece of evidence: nowhere in the code are the regions being ‘switch’ed on. This suggests that the way we use regions are not the same as the way we typically use enums.)
Looking closer at what regions actually are, they are nothing more than strings. Hence it is totally sufficient to just represent them as strings. And they are codable as well with no additional boilerplate needed. (If we go for the custom enum solution I suggested earlier, there would need to be some additional code handling the encoding/decoding of the custom case—not as elegant.)
I understand the problem of breaking changes (though technically we are still in RC stage and things can be changed if absolutely necessary). But the reason I still want to push for this is that I cannot use this library without hacks unless #18 is fixed, which would necessarily be a breaking change anyway. We might as well merge this PR into this batch of breaking changes. The cost would be lower than you expected.
… On 3 Jan 2019, at 10:58 PM, Ondrej Rafaj ***@***.***> wrote:
@rafiki270 requested changes on this pull request.
In Sources/S3Signer/Region.swift:
> @@ -4,68 +4,68 @@ import Foundation
/// AWS Region
public struct Region {
- /// name of the region, see RegionName
- public let name: RegionName
+ /// name of the region, see Name
+ public let name: String
this introduces a breaking change that won't bring much benefit to most users, could you please keep the original enum (it was Codable so you can store info about easily in a db for example) and just add .custom(String) as a case? I thought that's what was suggested on discord?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@dpgao so I have been thinking about this long and hard and think that we could create a new protocol called |
Why not make region type confirm to |
|
I guess that would work for me too ... :) |
|
@dpgao would you mind getting in touch on LiveUI Slack or Vapor Discord? I am @rafiki270 on both |
Currently region names are encoded as cases in an
enum. This is not flexible enough for situations where custom S3-compatible services are used. This pull request changes them to be represented by constant strings.