Add UI for adding and editing a custom list

[rablador]

The user should be able to create and edit a new custom list from the UI.

To test, simply add either AddCustomListCoordinator or EditCustomListCoordinator to some button action the UI somewhere, eg.

let coordinator = AddCustomListCoordinator(
    navigationController: CustomNavigationController(),
    customListInteractor: CustomListInteractor(repository: CustomListRepository())
)
coordinator.start()

coordinator.didFinish = {
    coordinator.dismiss(animated: true)
}

---

This change is 

[linear]

IOS-513 Add UI for editing a custom list

[buggmagnet]

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[acb-mv]

Reviewed 12 of 12 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift line 98 at r2 (raw file):

        tableView.deselectRow(at: indexPath, animated: false)

        dataSource.itemIdentifier(for: indexPath).flatMap { item in

flatMap (a functional operation that returns a value) used purely for side-effects can look unclear (a bit like map used in place of for _ in _). A more idiomatic alternative would be:

if let item = dataSource.itemIdentifier(for: indexPath) {
  didSelectItem?(item)
}

---

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 26 at r2 (raw file):

extension SettingsValidationErrorConfiguration: Equatable {
    static func == (lhs: SettingsValidationErrorConfiguration, rhs: SettingsValidationErrorConfiguration) -> Bool {
        lhs.errors.map { $0.errorDescription } == rhs.errors.map { $0.errorDescription }

Would it be possible to make SettingsValidationErrorProtocol conform to Equatable? Otherwise we have classes which have their own equality tests for types they contain, which could introduce bugs in the future.

[rablador]

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

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift line 98 at r2 (raw file):

Previously, acb-mv wrote…

flatMap (a functional operation that returns a value) used purely for side-effects can look unclear (a bit like map used in place of for _ in _). A more idiomatic alternative would be:

if let item = dataSource.itemIdentifier(for: indexPath) {
  didSelectItem?(item)
}

Waiting to resolve a discussion about the same thing in another PR.

---

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 26 at r2 (raw file):

Previously, acb-mv wrote…

Would it be possible to make SettingsValidationErrorProtocol conform to Equatable? Otherwise we have classes which have their own equality tests for types they contain, which could introduce bugs in the future.

I tried, but I can't get SettingsValidationErrorConfiguration to conform to equatable without custom equality tests anyway.

[buggmagnet]

Reviewed 11 of 12 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift line 54 at r2 (raw file):

        var snapshot = dataSource.snapshot()

        validationErrors.forEach { error in

nit
Right now there's only 1 case for CustomListFieldValidationError which is .name

So we could simplify this (at some cost for readability I admit) to
snapshot.reloadSections(validationErrors.map { _ in .name })

I think what begs the question is : if there's only one case for the error, does it make sense to store it as an enum ?
Here it makes the code more indirect (need to swich over cases)

This discussion is probably moot if we plan to have more than one case in the errors, but otherwise, maybe I want to ask if we couldn't turn CustomListFieldValidationError into a struct instead ?

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListItemIdentifier.swift line 54 at r2 (raw file):

    static func fromFieldValidationErrors(_ errors: Set<CustomListFieldValidationError>) -> [CustomListItemIdentifier] {
        errors.compactMap { error in

nit
Same discussion as above, we could simplify this to errors.compactMap { _ in .name } ?

---

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 26 at r2 (raw file):

Would it be possible to make SettingsValidationErrorProtocol conform to Equatable?

Long story short : We shouldn't, it's usually not a good idea. This article goes into great lengths to explain why.

It looks like to me that SettingsValidationErrorProtocol is not needed. The Error protocol already does all the required things.

Having CustomListFieldValidationError implement both SettingsValidationErrorProtocol and LocalizedError is redundant.

If you get rid of CustomListFieldValidationError and declare errors as such var errors: [CustomListFieldValidationError] = []
You get Equatable on SettingsValidationErrorConfiguration for free

---

ios/MullvadVPNTests/CustomListRepositoryTests.swift line 74 at r2 (raw file):

        repository.update(streaming)

        let gaming = try XCTUnwrap(repository.create("PS5", locations: [

⚠️ Initialization of immutable value 'gaming' was never used; consider replacing with assignment to '_' or removing it

What are the calls to update meant to do ?
If I remove them, the test still passes.

[rablador]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv and @buggmagnet)

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift line 54 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Right now there's only 1 case for CustomListFieldValidationError which is .name

So we could simplify this (at some cost for readability I admit) to
snapshot.reloadSections(validationErrors.map { _ in .name })

I think what begs the question is : if there's only one case for the error, does it make sense to store it as an enum ?
Here it makes the code more indirect (need to swich over cases)

This discussion is probably moot if we plan to have more than one case in the errors, but otherwise, maybe I want to ask if we couldn't turn CustomListFieldValidationError into a struct instead ?

I've been going back and forth on this. First I thought we should just simplify things and basically "hard code" the error handling and only care about name input. But that felt a little inelegant. Now we at least have a general error handling and can add things if we want (the final designs aren't set, so who knows). Either way, we can do what you propose, since that's on the consumer side. snapshot.reloadSections([.name]) would be even simpler.

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift line 98 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Waiting to resolve a discussion about the same thing in another PR.

Changed

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListItemIdentifier.swift line 54 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Same discussion as above, we could simplify this to errors.compactMap { _ in .name } ?

If we want a general error handling I think we shouldn't make changes like this on the "producer" side.

---

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 26 at r2 (raw file):

Previously, buggmagnet wrote…

Would it be possible to make SettingsValidationErrorProtocol conform to Equatable?

Long story short : We shouldn't, it's usually not a good idea. This article goes into great lengths to explain why.

It looks like to me that SettingsValidationErrorProtocol is not needed. The Error protocol already does all the required things.

Having CustomListFieldValidationError implement both SettingsValidationErrorProtocol and LocalizedError is redundant.

If you get rid of CustomListFieldValidationError and declare errors as such var errors: [CustomListFieldValidationError] = []
You get Equatable on SettingsValidationErrorConfiguration for free

Removed the protocol in favor of the concrete type.

---

ios/MullvadVPNTests/CustomListRepositoryTests.swift line 74 at r2 (raw file):

Previously, buggmagnet wrote…

⚠️ Initialization of immutable value 'gaming' was never used; consider replacing with assignment to '_' or removing it

What are the calls to update meant to do ?
If I remove them, the test still passes.

Lost in translation when I updated the tests. Removed them.

[buggmagnet]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListItemIdentifier.swift line 54 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

If we want a general error handling I think we shouldn't make changes like this on the "producer" side.

Up to you, it ultimately depends on whether we intend to expand CustomListFieldValidationError but I also want to say
"YAGNI, until you do, then you can make the change"

---

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 25 at r3 (raw file):

extension SettingsValidationErrorConfiguration: Equatable {
    static func == (lhs: SettingsValidationErrorConfiguration, rhs: SettingsValidationErrorConfiguration) -> Bool {

You can also remove the entire extension, it's not needed to implement Equatable.

If theElement type of an array is Equatable the array itself is too, and since all the types in SettingsValidationErrorConfiguration are Equatable it -- itself -- is too.

[rablador]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @acb-mv and @buggmagnet)

---

ios/MullvadVPN/Coordinators/CustomLists/CustomListItemIdentifier.swift line 54 at r2 (raw file):

Previously, buggmagnet wrote…

Up to you, it ultimately depends on whether we intend to expand CustomListFieldValidationError but I also want to say
"YAGNI, until you do, then you can make the change"

As discussed f2f, we'll leave it like this until we have the final designs.

---

ios/MullvadVPN/Coordinators/Settings/SettingsValidationErrorConfiguration.swift line 25 at r3 (raw file):

Previously, buggmagnet wrote…

You can also remove the entire extension, it's not needed to implement Equatable.

If theElement type of an array is Equatable the array itself is too, and since all the types in SettingsValidationErrorConfiguration are Equatable it -- itself -- is too.

Done.

[buggmagnet]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acb-mv)

[buggmagnet]

Reviewed 10 of 10 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv)

[acb-mv]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions