Split rule object_literal into image_literal and color_literal

[Jeehut]

I just implemented a custom rule to prevent that any of my coworkers use the UIImage(named: "...") initializer since there's the #imageLiteral now. It looks like this:

dynamic_image_reference:
    included: ".*.swift"
    regex: 'UIImage\(\s*named\s*:\s*\"'
    name: "Dynamic Image Reference"
    message: "Don't use dynamic image references via strings – use Xcode image literals instead."
    severity: warning

Then I naturally thought "why is this not part of SwiftLint anyways?" and looked it up to find the object_literal rule added via #1067. I also read the discussion there which was about adding the rule by default or not. The consensus was that #imageLiteral could be turned on by default but there's problems with #colorLiteral since some people don't use it – like we do, too. We use SwiftGen instead, which we find better since we can define the set of colors to use in a single file.

My question now is: Why not split the object_literal rule into two, namely color_literal and image_literal and turn on image_literal by default?

[marcelofabri]

We use SwiftGen instead, which we find better since we can define the set of colors to use in a single file.

That could be said about imageLiterals too.

I'd be in favor of making it configurable to just check one or another, but I still think it shouldn't be a default rule.

[Jeehut]

That could be said about imageLiterals too.

I disagree with this one. Technically you're right, since we used the images feature from SwiftGen before Xcode 8 was released. But since Xcode 8 there's no reason to stick to the SwiftGen feature as there's a new "single file" with all images: The Image Assets. Actually what SwiftGen does for images is exactly what the imageLiterals now do – it's an exact replacement. Both search the project for images and provide them in a way the compiler can auto-complete and ensure the resources are actually there.

But when it comes to colors, it's different. With SwiftGen you have to add an additional file, e.g. named Colors.txt and define your colors there. This is not a feature that colorLiteral can cover right now.

And there's one more difference between colors and images: We usually try to have as few colors as possible in a project, which I think is the goal for most developers and a best practice. That's not true for images though: You usually add more and more images with every new feature.

[marcelofabri]

But since Xcode 8 there's no reason to stick to the SwiftGen feature as there's a new "single file" with all images: The Image Assets. Actually what SwiftGen does for images is exactly what the imageLiterals now do – it's an exact replacement. Both search the project for images and provide them in a way the compiler can ensure the resources are actually there.

Not exactly, because if you rename/remove an image, compilation would fail with SwiftGen, but #imageLiteral would silently fail.

[Jeehut]

Making it configurable suits me, too, by the way. I just want to throw out my custom rule and use something more community-approved. So if I can turn the one on and the other off with the object_literal rule, I'd be okay with that, too. But I'm still not convinced why enabling imageLiterals would hurt any project when a rule like trailing_whitespace is on by default (I had to tell two of my coworkers right in the last two hours to turn on the Xcode feature "Include white-space only lines" in the trimming options) – that rule really causes many issues and I've seen many projects turning it off. So if somebody really has a reason not to use imageLiterals (e.g. using Xcode 7) then they could just turn it off. But I think it's a best practice nowadays.

[Jeehut]

Not exactly, because if you rename/remove an image, compilation would fail with SwiftGen, but #imageLiteral would silently fail.

WTF – seems like you're right, I didn't know that and just wanted to prove it wrong, to see that it really doesn't show an error. 😄 So what's the point of imageLiteral then, really. Just auto-completion? How sad ... okay sorry, now I agree on not making it default. I'm shocked right now ... 😅

[marcelofabri]

I guess the literal is resolved in the compiler, which doesn't know about asset catalogs? But, yeah, it'd be awesome if they were able to check it.

Anyway, if you still think it'd be helpful to make it a configuration, that'd be welcome.

[Jeehut]

Now that the shock is over, here's what I now think:

It's true that imageLiteral does not have any compile time existence check – BUT: In my custom rule above I only expect the user to replace UIImage(named: "...") calls with the image literal. Other services like SwiftGen have different names for that first parameter, cause the type is not a String. For example in SwiftGen the call would look like this: UIImage(asset: .banana).

So, I'm taking back that you convinced me, I still think that as a replacement to UIImage(named: "...") there's good reason to make the rule turned on by default for imageLiterals. You cannot mistype the image name on the first addition.

Having that said, if you still really don't want this, I would also volunteer for implementing the option on the rule, as you suggested. At least that's what I read as an implication from your last sentence?

[starr0stealer]

@marcelofabri You just gave me another reason not to use #imageLiteral(s). No build error on rename/remove, ugh, seems like an oversight on Apples part. Though for me, simply being "cute and clever" is enough, just looks weird in the middle of source code. I blame the Emoji fad train. I prefer compile time errors from R.swift, and its automated aspect.

@Dschee To me, files created by SwiftGen don't belong in the Source directory of the project. Should be handled similar to how it is with R.swift, i.e the file(s) go in the $SRCROOT and in the case of SwiftGen a subfolder to reduce clutter in the root of the project. These are "generated" files, created by a 3rd party tool, not directly authored by a developer, therefor are not true source code. Placing these files outside of the main Source directory removes any warnings those generated files would produce from Linting. Generated files are generally outside of a developers control, and rarely should be manually edited.

For example a folder structure like below, with SwiftLint only processing the "Source" directory.

  <root>
  | _ "R.generated.swift"
  | _ "<SwiftGenFilesFolder>/.."
  \ _ "Source/.."

This request sounds more like trying to make SwiftLint conform to the needs of a third party tool, not the language itself. Conformity like this isn't something a Linter is meant for. I believe that if someone is going to use one #*Literal odds are they will use the others.

My vote is that object_literal should remain opt-in, at least until Apple adds compile time errors when the Asset Image is removed/renamed. It's a subject I feel is very opinionated, some like seeing the fancy icons in the editor, others don't. Personally I don't like seeing them in raw source or the editor.

Though I don't really see much harm in adding configurable properties to the Rule, i.e. something like below.

object_literal:
  enable_image: false
# or
  enable_color: false

[Jeehut]

@starr0stealer About SwiftGen and the inclusion into the Source folder: I actually put it there, but that doesn't mean SwiftLint is running with the files. We do have a best practice to put all files generated by SwiftGen into a folder named "Constants" within the "Sources" folder (see here). This folder we ignore (see an example .swiftlint.yml). So, that's not a problem and it's not at all the reason for this request. I think there must be a misunderstanding on your side here somewhere.

Also I think you see the #imageLiteral in a wrong way: So small as the images are within the code, I highly doubt that the rationale behind introducing it (for the images at least) was the fact that you can see them within code. I think the preview information is much more useful in the #colorLiteral – which I don't use for other reasons though.

The rationale behind #imageLiteral is (or at least should be) that you prevent any typos at the moment you write the code of loading an image asset. As I learned here, this is never checked again later on after first adding the literal, but that doesn't mean that the literal is useless. It is still much better than doing a "UIImage(named: StringLiteral)" – where you're still using a literal (the StringLiteral), but one without auto-completion. So there's absolutely no harm in using the imageLiteral. It simply adds auto-completion to UIImage(named:) – that's all it is, basically. At least no one (including you) could tell me about a disadvantage compared to UIImage(named: StringLiteral).

[starr0stealer]

@Dschee No misunderstanding on my part, I seen you don't use the #colorLiteral and assumed it was already ignored by SwiftLint in your configuration.

If I gave the impression that I felt Literals were useless, I do apologize, not my intention. I merely expressed my dislike of them, I use R.swift because it provides a centralized "API" for all Assets within a project, something I felt should be part of the Xcode/iOS platform (like how Android does it). With R.swift you get instant compile time errors if any asset name has changed.

When Apple announced the Literals, the main keynote they talked about was about seeing the Asset inline within the code, seeing them quickly gave the author validation they selected the right asset. Other keynotes they suggested this feature added was brevity and speed. Literals take up less space, and have auto-complete. Another feature of the Literals is that you can drag/drop the asset onto a line of code.

Testing the code via running the App is the main way of ensuring no typo was made, if you see the image you typed the name of the Asset correctly.

Best Practices are a collection of opinions, sometimes held with greater merit depending on the origin, i.e. from a Languages maintainer/author, or from a well respected firm/group.

In my opinion, storing files generated within "Source/Constants" is not best practice for a few reasons. Main reason, the most accepted consensus of generated code is that it belongs in a build task, not source control, it is not authored by a developer but by a tool. Another reason I see, is the placement into the "Constants" folder, as the project grows you may author, by hand, file(s) that have Constant variables, now you will need to maintain the SwiftLint configuration to only ignore generated files.

But I digress, opinions above isn't the desired topic here.

The subject at hand is about two main topics. Splitting ObjectLiteralRule into multiple Rules, and defaulting some but not others.

To touch the first topic, splitting into multiple rules. Having more rules isn't always the best route, mostly from a maintainability perspective. Splitting these would open up a chance to violate DRY, or the need to add a Helper which introduces more code to be maintained. Rules should be designed in a more general sense, for this one the subject of #*Literal(s). If split into individual rules, as Apple adds more literals, that is more and more Rules needed to be added one by one, unneeded complexity.

Then the topic of defaulting some but not all. That enforces a major opinion in the toolkit, why should one be treated "more useful" than the others.

Instead, introducing configuration into a single rule provides both standpoints an acceptable solution. Those that have zero interest in using Literals can continue to not opt-in, while those that would like to use some by not all can disable the ones they do not want to use.

[marcelofabri]

I still think the best solution here would be supporting a configuration and keeping the rule as one (and opt-in). I just don't think there's enough consensus in the community for making it a default rule.

Also, this rule is more useful when a person/team is using Xcode, but there are people who just use other IDEs or text editors.

[Jeehut]

I just implemented said option in #1605.