[Technical] Add SwiftGen for assets

[fredpi]

This PR adds SwiftGen for assets and thereby lays the foundation for #30.

As there are currently multiple ways assets are used within the code, I didn't actually change the asset access mechanism yet – it's probably better if the core team does it.

To go on, I suggest a few things:

• Also use SwiftGen for storyboards:

ib:
  inputs: .../Folder
  outputs:
    - templateName: scenes-swift4
      output: .../Resources/Generated/Storyboards.swift

• Also use SwiftGen for localizations, combined with BartyCrouch, as shown in this project template.
• Use a custom template for SwiftGen assets like this one. It would be best, if all image assets could be accessed using ~Images.folderName.folderName.imageName and all color assets using ~Colors.folderName.folderName.imageName

[ChristianKienle]

@fredpi Thank you very much for this PR. I just wanted to let you know that I am now looking into this PR which includes testing it locally and trying to figure out if there is anything else that needs to be done.

The only concern I have at the moment - without having looked at it - is that there are currently 13+ iOS developers working on this project + several developers from the open source community. If this changes the workflow significantly we may have to document that and make everyone aware about the changes. That being said I am now starting the review and I am pretty curious what I will find once I open the box. :)

[ChristianKienle]

@inf2381 I believe our CI is not being executed for PRs coming from other repos. It this done intentionally?

[ChristianKienle]

@fredpi I now had a look at the PR. I think that we have to also adjust the circleci config.yml file so that swiftgen is installed before actually building the app. We also have to consider our internal CI landscape. I will ask around if this might cause any issues and report back to you.

I have also assigned @inf2381 (aka Mr. CI) to make him aware about what is happening.

[Jeehut]

@ChristianKienle Would love to see SwiftGen in this project as well, prevents many bugs and errors! Also I wonder why we are not using GitHub Actions for this regarding CI? @inf2381

[Jeehut]

@fredpi Why not build-script step that automatically runs SwiftGen on each build?

[fredpi]

@fredpi Why not build-script step that automatically runs SwiftGen on each build?

@Jeehut Do you mean a run-script phase in Xcode? I have added it, maybe you overlooked it ;)

Or do you mean something else?

[Jeehut]

Ahh, you're totally right of course, I overlooked it. Looks good, thank you! 👍

[fredpi]

@fredpi I now had a look at the PR. I think that we have to also adjust the circleci config.yml file so that swiftgen is installed before actually building the app. We also have to consider our internal CI landscape. I will ask around if this might cause any issues and report back to you.

I have also assigned @inf2381 (aka Mr. CI) to make him aware about what is happening.

@ChristianKienle Thanks for the suggestion, but I think that would be rather counterproductive:

Every developer that adds news assets should build afterwards (automatically causing SwiftGen to update the Assets.swift file), then commit both the new assets and the updated Assets.swift file. The CI, in contrast, should not manipulate files before / while building.

Of course, this setup will yield a warning within the CI (that SwiftGen isn't installed), but I guess that won't be a problem with the current CI configuration that doesn't fail immediately on warnings.

[ChristianKienle]

@fredpi I now had a look at the PR. I think that we have to also adjust the circleci config.yml file so that swiftgen is installed before actually building the app. We also have to consider our internal CI landscape. I will ask around if this might cause any issues and report back to you.
I have also assigned @inf2381 (aka Mr. CI) to make him aware about what is happening.

@ChristianKienle Thanks for the suggestion, but I think that would be rather counterproductive:

Every developer that adds news assets should build afterwards (automatically causing SwiftGen to update the Assets.swift file), then commit both the new assets and the updated Assets.swift file. The CI, in contrast, should not manipulate files before / while building.

Of course, this setup will yield a warning within the CI (that SwiftGen isn't installed), but I guess that won't be a problem with the current CI configuration that doesn't fail immediately on warnings.

Gosh you are right. The CI does not need it. :D I really need more sleep lol.

Okay so then the CI is not a concern. Then this PR is fine with me.

So the plan is, if this gets merged to then actually start using Assets.swift in our code base - right?

[ChristianKienle]

@ChristianKienle Would love to see SwiftGen in this project as well, prevents many bugs and errors! Also I wonder why we are not using GitHub Actions for this regarding CI? @inf2381

We are using CircleCI because it was the first CI platform to have the Xcode beta version we needed back then...

[Jeehut]

@ChristianKienle Thank you for the fast review and responses. The Beta explanation makes sense! 👍 Looking forward for this getting merged. I think as a subsequent PR @fredpi might want to migrate at least the more trivial uses of dynamic strings to SwiftGen.

@ChristianKienle What is your opinion on also using BartyCrouch for fixing multiple potential issues with translations? After this is merged, @fredpi or I could continue with that as he has pointed out in his first post in this thread.

[fredpi]

@ChristianKienle Thanks for the review! 👍

I've made some changes to this PR:

• SwiftGen is now also used for Storyboards – rendering the custom AppStoryboard type useless. While adding SwiftGen for storyboards I found a lost storyboard file (Main.storyboard) that isn't used anymore, so I removed it.
• A custom template is now used for the asset generation – allowing for faster access and enabling namespacing by default (resulting in code like Colors.SemanticColors.medium or Assets.EnSettings.iconsBluetooth). Also, all colors and images of a catalog can be accessed easily, e. g. using Colors.allColors.
• I have added custom SwiftLint rules that enforce the use of SwiftGen. Currently, they are still commented out.

As from my side, this can be merged now.

The next step would be to migrate the existing use to SwiftGen (+ enable the new SwiftLint rules). This can be done step by step, starting with storyboards, then colors and finally images. The latter may be a bit more time-intensive because the namespace must be looked up for each image.

Also, SwiftGen & BartyCrouch work quite well together for localizations, so those could also be implemented as @Jeehut suggested.

[ChristianKienle]

@fredpi Thank you very much for the changes. I will review this PR again - shortly.

@Jeehut & @fredpi To be honest I have never used BartyCrouch. Before I can make any qualified comment I would have to look at it which I will do. Not sure if I will be able to check it out today though...

[Jeehut]

@ChristianKienle BartyCrouch works like magic without the need that anyone learns something new, it actually just fixes gaps in Xcode and makes things work as expected. For example, when you write NSLocalizedString("new_key", comment: "") in code, it will automatically add the key to the Localizable.strings files for all languages. Also, whenever any new labels or buttons are added or removed from Storyboard files, it automatically syncs the translations of those Storyboards to add/delete the keys. Once we configured it, you won't need to do anything specific, just install it on your machine and you're good to go.

But having that said, it should for sure be a separate PR because SwiftGen is already useful without BartyCrouch, so we can move the BartyCrouch discussion to it's own PR.

@fredpi Instead of SwifLint custom rules (which I don't like because they neither support testing/validation nor autocorrection), could we use AnyLint which I have already basically configured in #104?

I have also already prepared AnyLint checks for using SwiftGen in other projects, here they are:

    let swiftFiles: Regex = #"^(App|Tests|UITests)/Sources/.*\.swift$"#
    let generatedSwiftFiles: Regex = #".*/Generated/.*\.swift|.*/SwiftGen/.*\.swift"#

    // MARK: DynamicColorReference
    try Lint.checkFileContents(
        checkInfo: "DynamicColorReference: Don't use dynamic color references – use SwiftGen & Color instead.",
        regex: #"UIColor\(\s*named:\s*\""#,
        matchingExamples: [#"UIColor(named: "primary")"#],
        nonMatchingExamples: ["Colors.primary.color"],
        includeFilters: [swiftFiles],
        excludeFilters: [generatedSwiftFiles]
    )

    // MARK: DynamicStoryboardReference
    try Lint.checkFileContents(
        checkInfo: "DynamicStoryboardReference: Don't use dynamic storyboard references – use SwiftGen & StoryboardScene instead.",
        regex: #"UIStoryboard\(\s*name:\s*\""#,
        matchingExamples: [#"UIStoryboard(name: "LoginViewController")"#],
        nonMatchingExamples: ["StoryboardScene.Login.loginViewController.instantiate()"],
        includeFilters: [swiftFiles],
        excludeFilters: [generatedSwiftFiles]
    )

    // MARK: DynamicStringReference
    try Lint.checkFileContents(
        checkInfo: "DynamicStringReference@warning: Don't use dynamic localization string references via code strings – use SwiftGen & L10n instead.",
        regex: #"NSLocalizedString\s*\("#,
        matchingExamples: [#"NSLocalizedString(@"Test", comment: "");"#, #"NSLocalizedString("Test", comment: nil)"#],
        includeFilters: [swiftFiles],
        excludeFilters: [generatedSwiftFiles]
    )

[haosap]

Hi guys,
Thanks for your discussion here. SwiftGen is not a new concept, as the idea is very similar to the Android R file. Is there some one who can tell me what's the benefit it can bring in to the project? If it's possible, personally, I prefer the following way to organize the assets:

 enum BluetoothState {
  case on
  case off
  private var imageName: String {
   switch self {
   case on: return "Illu_Bluetooth_Off"
   case off: return "Illu_Bluetooth_On"
   }
  }
  var image: UIImage { UIImage(named: imageName) }
 } 

I think this is more logic.

[Jeehut]

@haosap SwiftGen is a generalized approach to fixing any resource loading problem which is by default done by manually referincing dynamic strings in iOS development. Unfortunately, the default method is very prone to errors like typos, referencing no longer available or renamed files. Because Xcode neither has a built-in check for the existence of the files and also doesn't provide autocompletion, errors are quite common, especially if the project is open source and many different people work on it.

In the end, yes, Android Studio for example has a similar concept built-in, and yes it's possible to set up a different solution like a custom one (that you suggested) or R.swift. But as SwiftGen is by far the most used solution here and many iOS developers know it, it's IMHO the best solution to tackle the problem of error-prone autocompletion-free nature of loading resources in iOS development without creating unnecessary obstacles for new contributors.

[fredpi]

@ChristianKienle I have just rebased to fix conflicts. The tests pass so I think there's nothing holding back this PR from getting merged.

Just one last addition to this PR: I have added SwiftGen for strings. I didn't do it until now because I thought it would be better if it would be in a PR together with BartyCrouch, but for me it seems like the general expectation is that this PR already adds SwiftGen for strings. Unfortunately, the keys for the strings use an underscore pattern (main_screen_some_string) instead of a combination of points and underscores (~ main_screen.some_string) that would cause SwiftGen to better structure the strings (resulting in code like L10n.MainScreen.someString instead of L10n.mainScreenSomeString).

@Jeehut Yes, using AnyLint would be better, but maybe this PR should still implement SwiftLint rules and AnyLint migration should be discussed in #104.

@haosap Actually a pattern like Images.BluetoothState.on is possible with SwiftGen using namespacing (taking into account the group structure in the assets catalog Images.xcassets). However, as the asset catalogs are currently configured not to use namespacing themselves, the last name must be unique (Images.BluetoothState.bluetoothStateOn). That could be changed but one must be extremely cautious to properly update all the names referenced via storyboards – I wouldn't do it now that we don't have much time for it. Also, I'm wondering why there are multiple images assets catalogs; is this for performance reasons? I guess it would be easier to just have Images.xcassets and Colors.xcassets.

[Jeehut]

From @haosap:

Hi,
Could you please give me some example? What kind of MANY bugs will bring in this project besides Typo, without using the SwifGen?
Thanks.

@haosap Firstly, I don't know why you edited my comment instead of clicking "Quote reply". I assume you are searching for a thread-like answer method like in other tools like Slack, which will be coming to GitHub soon (I think this summer): https://help.github.com/en/github/building-a-strong-community/about-team-discussions

Apart from that, let me answer your question. Firstly, the way you ask your question seems to already include a logical issue: Your question implies that only one bug type ("Typo") can not lead to "many bugs", which is untrue. There can be thousand typos in the repo which would be thousand bugs but all of them consisting of one bug type.

Having that said, I will also quote myself because I already mentioned 3 bug types that are prevented by using SwiftGen:

Unfortunately, the default method is very prone to errors like typos, referencing no longer available or renamed files.

So not only typos are fixed, but also if one of the resources is removed (e.g. an image, a string or a color) then the project won't build anymore which makes clear that there's an issue. Without SwiftGen, it will continue to build, leaving to a user-error after shipping.

Or if one of the resources was renamed (this usually happens in the last steps before a release where things are refactored to make things more consistent after a fast-progress period) then the same thing happens, SwiftGen again helps.

In practice, we have prevented around 1-2 bugs shipping per 2-week sprints to users in teams of 3 developers. Here, we are many more developers so I expect more bugs, but of course that also means someone could check that all resources are still available before making a release – or we just use SwiftGen and save that poor guy from doing these manual checks.

[fredpi]

@ChristianKienle I rebased to fix conflicts and to adapt to the new asset catalog structure.

In an additional commit, I renamed Assets.xcassets to Images.xcassets and ena-colors.xcassets to Colors.xcassets. If you're not okay with that change, I can withdraw it. But it enables code like Images.someImageInTheAssetsCatalog (instead of Assets.someImageInTheAssetsCatalog) and Colors.someColorInTheEnaColorCatalog (instead of EnaColors.someImageInTheAssetsCatalog). Such an effect may also be achieved by modifying the SwiftGen-xcassets.stencil - just let me know what to do.

[fredpi]

@ChristianKienle As my asset catalog renaming caused a bunch of conflicts, I now switched to the different approach (modifying the SwiftGen template), that only renames the asset catalogs in the Assets.swift file.

Also, as rebasing got trickier every time, I squashed the previous changes into one commit.