[IRL-10685] Make some suggested changes to style guide

[benjaminbreier]

Summary

I applied some of the feedback received on the test run of the Airbnb style guide to be reviewed.

How I addressed each feedback

Each line should have a maximum column width of 100 characters
⚠️ We're currently doing 120, but I don't mind this too much

I changed it to 120. I think its more important that we have a max, than what the max is.

Use 2 spaces to indent lines
🛑 I disagree with this. Spaces are obviously more visually consistent, but we would be forcing a consistency that might be inconvenient for some folks (see https://www.reddit.com/r/javascript/comments/c8drjo/nobody_talks_about_the_real_reason_to_use_tabs/). We kept 4 spaces because it was already the case to avoid massive changes, but if we do change indentation then it should be to tabs

I removed this entirely.

Use PascalCase for type and protocol names, and lowerCamelCase for everything else
⚠️ We already do this, but with exceptions: properties with no semantics to their names, or better their semantic is the name, should not be constrained by this (DTOs are a good example, check out the API models in our repo)

This is not automated, so I changed the wording in the README to reflect the feedback.

Names should be written with their most general part first and their most specific part last
⚠️ I think that it's common practice to leave the type part of the name last. Apart from that, not opposed to it

I think based on the existing wording, this already fits just fine, so I left it unchanged.

Event-handling functions should be named like past-tense sentences
⚠️ I don't necessarily agree with this (handle prefix for example gives a good hint about the nature of the method), but I can live with it

Yea, I agree, handle does work in many cases. None of the naming is automated so it can be taken as a strong suggestion :)

Avoid *Controller in names of classes that aren't view controllers
⚠️ Kinda agree, but we followed the previous naming style here and didn't want to cause massive changes for it

Totally agree, modified the wording to reflect this.

Don't include types where they can be easily inferred
⚠️ Agree, but with the exception of properties

I left this in. My rationale is that in the case of properties, the type is always included on the right hand side of the declaration when it can be omitted from the left, so it is never ambiguous. I hope that's alright

Separate long function declarations with line breaks before each argument label and before the return signature

🛑 I agree with it apart from where to put the return signature: this style gives no clear distinction between arguments and the return signature and it's harder to spot if a function has a return or not, especially when effects (throws, async) are included. That's why we do follow this rule but put the return signature on the previous indentation level and with the closing brace on the same line

I've done my best to modify the rule appropriately based on this feedback.

Infix operators should have a single space on either side
⚠️ Yes but I would apply this to range operators too

I actually disagree, but I don't mind much and went ahead and changed this to apply to range operators.

Avoid performing any meaningful or time-intensive work in init()
⚠️ I generally agree, and totally agree if this means to not wait for that work in init. But I think in some cases it makes sense to trigger it (though I can be okay with enforcing this)

Yes, and this kind of style guide rule starts to creep into "best practices" recommendations. It is generally correct, but there could be exceptions. I don't think it is automated, either. We could leave it in as a guideline, or remove it entirely.

Extract complex callback blocks into methods
⚠️ I agree with the general sentiment, but I'm not sure that it should be a rule. Some times it might make sense to extract part of it to well-defined logical operations instead of all of it. async/await greatly reduces the need for this anyway

It does. I agree with the thrust-- reading through someone else's code to figure out what it is doing is much more difficult when we have to leave the context to follow a dependency chain to see what a closure is doing unnecessarily. There will always be times where this is required, but I think this is another case of a "mostly true guideline".

Access control should be at the strictest level possible
⚠️ Yes, but sometimes you design APIs before you use them so you can prepare the access control for that usage. If we append "for the intended usage" at the end I agree

I think that's reasonable. This is a small concern for us now, it becomes much larger when you are dealing with a very large codebase (1m+ lines of swift) and access control begins to have a meaningful impact on build times. I still believe the thrust is correct, and we could argue that your exception still follows the wording of the rule. I added "for the intended usage" to the README.

Use Swift's automatic enum values unless they map to an external source
🛑 I actually agree with the sentiment, but there's a but: when we have mappings to an external source, we make a separate type explicitly for that. Having the separate type, the fact that changing it is dangerous is already explicit, so there's no need to be explicit about the rawValues

If I understand correctly, you are saying that in these situations you write two enums, one for the API's data model and one for the client's, and some glue to convert between the two, thus making the external mapping explicit. I think that's a reasonable approach to solve the same problem, and modified the wording accordingly.

Never use the default case when switching over an enum
⚠️ If the rule is explicitly about a single enum, then ok (even a big one is still doable). Sometimes we do switch on tuples of enums and not having a default is impractical

Yep, once you are switching over a combination this is untenable. I modified it to be specific to single enums.

Alphabetize and deduplicate module imports within a file. Place all imports at the top of the file below the header comments. Do not add additional line breaks between import statements. Add a single empty line before the first import and after the last import

⚠️ Agree with most of this, but two things:
Doesn't make sense to have an empty line before the first import if there's nothing else before it (and it's our case)
I prefer putting imports for specific things (like SwiftUI live previews) next to where it's needed, but this can be ok

I removed the guidance about the empty line (could not find an automated rule that was enforcing it). I think we'll keep preferring imports at the top of the file for now.

Declarations that include scopes spanning multiple lines should be separated from adjacent declarations in the same scope by a newline
⚠️ Newlines are used to visually group sections of code, so if we have say 10 similar properties that are all single-line apart one it would feel weird to make that one break the grouping

Properties don't always include scopes spanning multiple lines, and when they do I think it is generally fine to give them some spacing. I agree that single line declarations of properties should not have spaces between them, and I believe that is not the intent of this rule.

Use // MARK: to separate the contents of type definitions and extensions into the sections listed below, in order

🛑 I agree with the spirit, but disagree with the grouping and/or ordering. What I've been pushing for is something like

type MyType {
  // Stored properties (split by context)
  // Lifecycle
  // `override`s, ordered by superclass hierarchy (most general superclass first)
}

// one `extension MyType` per access control of computed properties/methods
// one `extension MyType: MyProtocol` per conformance

This is tough. I do not mind the extension method of grouping code, although I think it serves effectively the same purpose as a pragma mark. That said, SwiftFormat provides the tooling to automate adding pragma marks and not grouping things into extensions. Would you be okay with letting it group into pragma marks as described and letting the extension division be optional or preferred?

Within each top-level section, place content in the following order

🛑 Kinda agree with giving an order, but not with the specific order. I would almost do the exact opposite of the proposed one

Also tough. As far as I could see, there is no way to customize the order of these. I have considered / argued about this particular order in the past and I'd be happy to discuss / justify it if that might help assuage your concerns. Apart from that, the question becomes: would you rather have an automatically enforced order, even if it isn't the exact order you'd have chosen yourself? Or would you forego the automatically enforced order for that reason. I prefer the former.

Add empty lines between property declarations of different kinds
⚠️ I don't mind too much, though I feel contextual grouping should be stronger than type of symbol

I didn't change anything related to this, I think you make a fair point, but contextual grouping isn't possible to automate in any way I can think of.

Computed properties and properties with property observers should appear at the end of the set of declarations of the same kind

⚠️ I did say before that I would put computed properties separate, so that doesn't apply here. For observers, I feel like this is artificial and the order shouldn't depend on how big the symbol is in code

I think its helpful to quickly parse what amounts to the interface of a class or struct and what it is, i.e. I can see that there are 5 properties injected into it at initialization and two computed properties which are derived from X, Y, and Z almost at a glance if the order is set up like this.