-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
base: main
Are you sure you want to change the base?
Indentation style #5144
Changes from all commits
c21f01d
89b5ab9
45b84aa
92983c3
bdf4735
513d9e5
2dfad93
5909311
5bbee51
9490124
e78e18d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
@ConfigurationElement(key: "per_file") | ||
private(set) var perFile = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got that. Perhaps this can be combined with This would have the pretty side effect, that the |
||
@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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should have a reasonable default, shouldn't it? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer the auto-generated There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my other suggestion about extending |
||
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 | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
wherein
cond2
might be indented by one tab and one space.In other words, is there any reason to make this optional?