Do not generate duplicate enum cases for segue IDs

[filwag]

No description provided.

[AliSoftware]

What about Segues that have different names but that could result in the same swiftIdentifier? Like Foo-Bar and Foo_Bar both generating the case name Foo_Bar…

[AliSoftware]

Note that this is also discussed in #7 where we discuss the various possible cases. Like:

• the fact that sometimes we need to generate the case anyway (and thus we need to append an index increment to it to make it distinct) because it actually doesn't represent the same Segue with the same identifier (like case Foo_Bar = "Foo-Bar" vs. case Foo_Bar2 = "Foo_Bar")
• The idea that this shouldn't be managed at the code level, but rather at the template level, because it depends on how the user chose to generate the swift identifier for that segue (maybe some people will use segue.identifier|swiftIdentifier but some other will use segue.identifier|swiftIdentifier|uppercase because they prefer having their cases all in uppercase of whatnot… and the important thing is that the generated final identifier is not a duplicate, so that FooBar and FOObar don't both generate case FOOBAR
• The fact that this logic should be applied to quite everything, not just segues but also images, colors, strings, …

[filwag]

Thanks a lot for pointing this out!

What if we use nested enums as associated values to disambiguate? Something similar to this:

/* storyboard segues:
   my_orange
   myorange
   my_Pineapple
*/

// generated enum:
enum Fruit {
  enum MyOranges: String {
    case my_orange = "my_orange"
    case myorange = "myorange"
  }

  case MyOrange(MyOranges)
  case MyPineapple

  func string() -> String {
    switch self {
    case .MyPineapple: return "my_Pineapple"
    case .MyOrange(let orange): return orange.rawValue
    }
  }
}

let pineapple: Fruit = .MyPineapple
let orange: Fruit = .MyOrange(.my_orange)

Please note that this PR solves a smaller issue that is a bit different from swfitIdentifier transformation ambiguity issue (#7). I'm simply filtering out exact duplications on an early stage, this is pretty similar to what happens for images (please see AssetsCatalogParser.swift:14).

[filwag]

My example above would not work if segue string includes dots.

[AliSoftware]

I don't like the nested enums approach, not having all the enums of the same storyboard at the same level (some at the root and some nested) seems inconsistent, and it doesn't feel intuitive at the call site.
Additionally, it would make the Stencil.Context quite complex to grasp and the stencil templates harder to write.

[AliSoftware]

What about using a Set<Segue> instead of a [Segue] here, so we get that for free and won't need the appendSegue private method then, letting that work to the Set?

[filwag]

Good point, changed it to Set<Segue>.

[AliSoftware]

Feels strange to only compare segueID.

What if the user wants to create a template that use both segueID and customClass to generate the case name?

Like some might prefer to have their template generating this: case {{segue.identifier|swiftIdentifier}}_{{segue.customClass|swiftIdentifier}} = {{segue.identifier}} and thus if two segues have the same identifier but different customClass that wouldn't generate a conflict, and the user won't have the code he expect to be generated.

That's another reason why I still feel this de-duplication should append at template level.

[filwag]

Yep, good catch. There were no mentions of customClass in default template, so I've just ignored it which is not right. I'll address this.

[AliSoftware]

I think for the sake of code consistency, migrating the Scene to a struct and the list of Scene into a Set could be nice?

[filwag]

Sure, I'll update this too.

[filwag]

Updated the PR according to the comments. My simple de-duplication now also takes customClass into account. It will still end up with duplicates for two segues with the same id but different custom classes when customClass is not used in the template. But there won't be duplicates when both params are equal. Other cases mentioned in #7 remain unresolved.

[AliSoftware]

Let me review that properly once I'm on my Mac before merging, but from there in GitHub LGTM ;)

[AliSoftware]

😂 dammit Objective-C habits ^^

[AliSoftware]

Hey @filwag

So I reviewed that code rn and I just miss a couple of things:

• Now that we adopted Sets, I think it'd be more logical to expose the storyboardScenes and storyboardSegues dictionaries as [String: Set<Scene>] and [String: Set<Segue>] directly, instead of converting them back to Array in the Parser; then do the conversion Array(scene) and Array(segue) here instead, for more consistency.
• Please also add an entry in the CHANGELOG.md to credit yourself 😉 (just add a ## Master entry at the top first)

[filwag]

Hi @AliSoftware, thanks for the review, I'll work on that :)

[filwag]

Updated. @AliSoftware, explicit Array(scenes) conversion here is not required, because sort() returns an Array.

[AliSoftware]

Merged via e85f7c1

Thanks again for your work on this!