Use string constants for region names

[dpgao]

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.

[dpgao]

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 notifications@github.com 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.

[rafiki270]

@dpgao so I have been thinking about this long and hard and think that we could create a new protocol called RegionConvertible: CustomStringConvertible which would any type which adheres to the protocol ... including string. The reason for this is that I am not a big fan of generalising custom typed properties to generic strings, ints, etc as it makes slightly less clear to the developer what should go in and what format ...

[dpgao]

The reason for this is that I am not a big fan of generalising custom typed properties to generic strings, ints, etc as it makes slightly less clear to the developer what should go in and what format ...

Why not make region type confirm to ExpressibleByStringLiteral? This way people can either use the property accessor notation (e.g. .usEast1) or just pass in a string literal, which would automatically be converted to have the region type.

[rafiki270]

I guess that would work for me too ... :)

[rafiki270]

@dpgao would you mind getting in touch on LiveUI Slack or Vapor Discord? I am @rafiki270 on both