Indentation style #5144
Indentation style #5144
Conversation
|
Can you add code example of your PR please ? |
|
Sorry, I don't know how this slipped through my radar. I didn't notice a response until now.
I'm not sure I understand what you mean. Are you asking for something like the rule description? |
| @ConfigurationElement(key: "preferred_style") | ||
| private(set) var preferredStyle = PreferredStyle.spaces | ||
| @ConfigurationElement(key: "tab_width") | ||
| private(set) var tabWidth: Int? |
There was a problem hiding this comment.
Should have a reasonable default, shouldn't it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @ConfigurationElement(key: "include_multiline_strings") | ||
| private(set) var includeMultilineStrings = false | ||
| @ConfigurationElement(key: "include_multiline_comments") | ||
| private(set) var includeMultilineComments = true |
There was a problem hiding this comment.
What is a good argument to exclude comments?
There was a problem hiding this comment.
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.
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?
| } | ||
| } | ||
|
|
||
| mutating func apply(configuration: Any) throws { |
There was a problem hiding this comment.
I would prefer the auto-generated apply implementation. Is there any specific reason for this custom version?
There was a problem hiding this comment.
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.
See my other suggestion about extending PreferredStyle to circumvent this special case.
| case spaces | ||
|
|
||
| func asOption() -> OptionType { | ||
| .string(rawValue) |
There was a problem hiding this comment.
Should be .symbol(rawValue).
| var violations: [StyleViolation] = [] | ||
|
|
||
| var fileStyle: ConfigurationType.PreferredStyle? | ||
| if configuration.perFile == false { |
There was a problem hiding this comment.
| if configuration.perFile == false { | |
| if !configuration.perFile { |
There was a problem hiding this comment.
I understand the irony of me arguing any style preferences on this project, but the reason I do this is for cognitive overhead while reading code. I used to always prefer the conciseness of the bang operator for testing conditions, but another programmer shared that they found reading new code much faster when there was a big ol' false staring you in the face instead of a tiny little !.
I'll go ahead and change it, but I just wanted to share my reasoning :)
| @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.
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.
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.
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.
| fileStyle = .tabs | ||
| confirmedFileStyle = .tabs | ||
| default: | ||
| queuedFatalError( |
There was a problem hiding this comment.
Prefer to report another violation instead of crashing.
There was a problem hiding this comment.
Oooh smart! Blind spot there. Thanks!
| identifier: "indentation_style", | ||
| name: "Indentation Style", | ||
| description: """ | ||
| Indent code using either tabs or spaces, not a chaotic mix. Can be configured to per file or project wide. |
There was a problem hiding this comment.
I had to smile when I read the "chaotic mix". Yet, I think it's a bit too offensive for a documentation. Better leave it out. 😉
| Indent code using either tabs or spaces, not a chaotic mix. Can be configured to per file or project wide. | |
| Code should be indented using either tabs or spaces. Can be configured to per file or project wide. |
There was a problem hiding this comment.
I had to smile when I read the "chaotic mix"
Mission accomplished :) I can now close this PR!
| private | ||
| func previousLineSetsNonstandardIndentationExpectation(_ currentLine: Line, in file: SwiftLintFile) -> Bool { |
There was a problem hiding this comment.
| private | |
| func previousLineSetsNonstandardIndentationExpectation(_ currentLine: Line, in file: SwiftLintFile) -> Bool { | |
| private func previousLineSetsNonstandardIndentationExpectation(_ currentLine: Line, in file: SwiftLintFile) -> Bool { |
(nit) spelling (fix) corrected PR number in changelog (nit) ironically didn't run swiftlint to see that I needed to fix some violations ... until now (fix) changelog values
* use `symbol` instead of `string` * `!boolean` instead of `boolean == false` * produce violation instead of fatalError * More professional description * readdressed line length on private func * document `tab_width` usage
mredig
force-pushed
the
indentation-style
branch
from
9ae28ba to
5bbee51
Compare
|
Hmm - Looks like there've been some substantial changes since this PR was made. Working on getting it to build and pass tests with the latest updates.... |
to build and pass tests
|
mredig
force-pushed
the
indentation-style
branch
from
60baf25 to
e78e18d
Compare
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.
I'll admit that I'm a little apprehensive about this PR because I feel like this feature should somehow already be here, but I cannot figure out how to use it if it is. Either way, I hope it's helpful!