Change AnyRelay.location from a String to a custom type

[acb-mv]

This defines a new type, REST.LocationIdentifier, which replaces the string values used in relay locations. These location values are strings of the format aa-bbb, so pre-parsing them into components makes sense. LocationIdentifier splits them into their components, which are accessible as its country and city attributes, though uses the underlying string for holistic operations (such as comparison, hashing and as a dictionary key).

---

This change is 

[rablador]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acb-mv)

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 41 at r1 (raw file):

            // code initialised from a literal in code, and
            // never from real-world input, so it'll have to do.
            fatalError("Invalid LocationIdentifier: \(value)")

Nit: I'm not against this, but would it make sense to let this init return an optional or throw instead? More boiler on call site, but perhaps also more "correct".

[acb-mv]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 41 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: I'm not against this, but would it make sense to let this init return an optional or throw instead? More boiler on call site, but perhaps also more "correct".

I considered that for that reason. The problem is, allowing LocationIdentifier to contain a null value would cancel one gain of introducing this type: making invalid states unrepresentable. I could have had it initialise to "aa-bbb" or something if it failed to parse, but that would just clutter the code with another code path, and I figured that requiring testing code to include properly formatted location identifiers (i.e., "xx-yyy" instead of "", as below) would be the lesser compromise.

[acb-mv]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 41 at r1 (raw file):

Previously, acb-mv wrote…

I considered that for that reason. The problem is, allowing LocationIdentifier to contain a null value would cancel one gain of introducing this type: making invalid states unrepresentable. I could have had it initialise to "aa-bbb" or something if it failed to parse, but that would just clutter the code with another code path, and I figured that requiring testing code to include properly formatted location identifiers (i.e., "xx-yyy" instead of "", as below) would be the lesser compromise.

(Note that this is for ExpressibleByStringLiteral, a protocol used only for initialising a type from string literals (as in foo.location = "aa-bbb". It is not intended to be called by user code, and there is no provision for failable initialisation or throwing.)

[rablador]

Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 32 at r1 (raw file):

}

// ExpressibleByStringLiteral: this allows literal assignments, like

nit
As much as I always appreciate comments, I think this one is a bit overkill,
i.e. we should leave comments when there is something obscure happening, or not immediately trivial to understand.
If we can just option-click something directly, the comment isn't very salient.

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 59 at r1 (raw file):

// Allow LocationIdentifier to code to/from JSON Strings
extension REST.LocationIdentifier: Codable {

nit
Why not declare Codable conformance directly in REST.LocationIdentifier ?
It feels odd to have to scroll down at the bottom of the file to find the init method of the type only to have to scroll back up to find how it's parsed.

[acb-mv]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 32 at r1 (raw file):

Previously, buggmagnet wrote…

nit
As much as I always appreciate comments, I think this one is a bit overkill,
i.e. we should leave comments when there is something obscure happening, or not immediately trivial to understand.
If we can just option-click something directly, the comment isn't very salient.

OK, removed.
I put the comment in to remind people looking at the code that that initialiser was for a system API not intended to be called from user code, and thus the fatalError in it was less of a code smell than it looks like.

---

ios/MullvadREST/Relay/LocationIdentifier.swift line 59 at r1 (raw file):

Previously, buggmagnet wrote…

nit
Why not declare Codable conformance directly in REST.LocationIdentifier ?
It feels odd to have to scroll down at the bottom of the file to find the init method of the type only to have to scroll back up to find how it's parsed.

I was following the common Swift practice of declaring conformances separately as extensions where they can be separated from the base definition. I'll move Codable above Hashable(which declares no initialisers).

[buggmagnet]

Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved