Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indentation style #5144

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,13 @@
* Rewrite `control_statement` rule using SwiftSyntax.
[SimplyDanny](https://github.com/SimplyDanny)

* Add rule `indentation_style`. Warns when tabs and spaces are mixed within the
same file and allows for setting your entire project to either tabs OR
spaces. Also allows for spaces to finesse placement at the end of a tabbed
indentation.
[Michael Redig](https://github.com/mredig)
[PR 5144](https://github.com/realm/SwiftLint/pull/5144)

* Add new `non_overridable_class_declaration` rule that triggers on `class`
function and variable declarations in final classes that are not final
themselves or private.
Expand Down
1 change: 1 addition & 0 deletions Source/SwiftLintBuiltInRules/Models/BuiltInRules.swift
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public let builtInRules: [any Rule.Type] = [
ImplicitReturnRule.self,
ImplicitlyUnwrappedOptionalRule.self,
InclusiveLanguageRule.self,
IndentationStyleRule.self,
IndentationWidthRule.self,
InertDeferRule.self,
InvalidSwiftLintCommandRule.self,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import SwiftLintCore

struct IndentationStyleConfiguration: SeverityBasedRuleConfiguration, Equatable {
typealias Parent = IndentationStyleRule

@ConfigurationElement(key: "severity")
private(set) var severityConfiguration = SeverityConfiguration<Parent>.warning
@ConfigurationElement(key: "include_multiline_strings")
private(set) var includeMultilineStrings = false
@ConfigurationElement(key: "include_multiline_comments")
private(set) var includeMultilineComments = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a good argument to exclude comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, it's because Xcode automatically indents multiline comments content with an additional space, even if you use tabs for indentation. As a tabs evangelist, even I don't always care to fix that since the smallest tweak can cause Xcode to reset that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If trailing spaces are only used to refine the indentation, but otherwise the file's default is respected by Xcode so that either spaces or tabs are used for the main chunk of the indentation, this seems like a normal case that should be supported anyway also in other areas, e.g.

if cond1,
   cond2 {
    // ...
}

wherein cond2 might be indented by one tab and one space.

In other words, is there any reason to make this optional?

@ConfigurationElement(key: "per_file")
private(set) var perFile = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this option. I tend to keep it simple for the first and only have a project-wide preferred setting instead of individual variants per file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that many projects I've come across have a hodgepodge mix. I included this so migration to project wide settings can be a little more gradual. Not to mention that Xcode allows setting tabs or spaces on a file by file basis, I don't want to cause violations in areas where teams may have already deliberately gone against the grain.

I can remove, but the work is already done!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got that. Perhaps this can be combined with preferredStyle in the sense of having spaces, tabs and automatic (or perFile) as options?

This would have the pretty side effect, that the apply method can be auto-generated.

@ConfigurationElement(key: "preferred_style")
private(set) var preferredStyle = PreferredStyle.spaces
/// Checks to make sure that in tab mode indentation, any spaces are only at the end and are, at most, tabWidth-1
/// in quantity, if tabWidth is set.
@ConfigurationElement(key: "tab_width")
private(set) var tabWidth: Int?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have a reasonable default, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better with a different name. This isn't for setting how many spaces a tab should consist of, it's to tell swiftlint how wide your tabs are, since I don't think that can be derived from anywhere...

The reason being, if you are using tab indentation, sometimes you use spaces to provide a little nudging after the tab indentation. This setting allows for no more than tabWidth - 1 spaces following any tab indentation.

I'll point out that, if you are using tabs in what I consider to be the most optimal way, you don't need this. However, forcing tabs projects to use tabs my way goes entirely against the philosophy of tabs usage in the first place, so this setting now exists.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also be required, if the we offer an automatic rewrite for the rule.

But I wonder, what happens if it remains nil. Then the rule would have to assume something, wouldn't it? Is it better to specify 2 or 4 as defaults right away then?


static let testTabWidth: [String: any Sendable] = ["tab_width": 4]
static let testMultilineString: [String: any Sendable] = ["include_multiline_strings": true]
static let testMultilineComment: [String: any Sendable] = ["include_multiline_comments": false]

@MakeAcceptableByConfigurationElement
enum PreferredStyle: String {
case tabs
case spaces
}

mutating func apply(configuration: Any) throws {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the auto-generated apply implementation. Is there any specific reason for this custom version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, I think it's for this bit:

if perFile == false {
	if let preferredStyle = configurationDict[$preferredStyle] as? String {
		self.preferredStyle = PreferredStyle(rawValue: preferredStyle) ?? .spaces
	}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other suggestion about extending PreferredStyle to circumvent this special case.

guard let configurationDict = configuration as? [String: Any] else {
throw Issue.unknownConfiguration(ruleID: Parent.identifier)
}

if let config = configurationDict[$severityConfiguration.key] {
try severityConfiguration.apply(configuration: config)
}

if let perFile = configurationDict[$perFile.key] as? Bool {
self.perFile = perFile
}

if perFile == false {
if let preferredStyle = configurationDict[$preferredStyle.key] as? String {
self.preferredStyle = PreferredStyle(rawValue: preferredStyle) ?? .spaces
}
}

if let includeMultilineStrings = configurationDict[$includeMultilineStrings.key] as? Bool {
self.includeMultilineStrings = includeMultilineStrings
}

if let includeMultilineComments = configurationDict[$includeMultilineComments.key] as? Bool {
self.includeMultilineComments = includeMultilineComments
}

self.tabWidth = configurationDict[$tabWidth.key] as? Int
}
}