Add option for opening_brace rule not to trigger with multiline statements
#5521
Conversation
|
@SimplyDanny Could you take a look at this when you have some time? It would benefit many that are used to adopting this style. I left the new option on by default just to check with the OSS projects, all the changes are the expected fixed violations. |
There was a problem hiding this comment.
I wonder why this new option does not apply likewise to guard and for (and maybe other statements). Any reason for that?
I left the new option on by default just to check with the OSS projects, all the changes are the expected fixed violations.
Very good! That helps a lot in assessment.
Source/SwiftLintBuiltInRules/Rules/Style/OpeningBraceRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Style/OpeningBraceRule.swift
Outdated
Show resolved
Hide resolved
| @@ -8,4 +8,6 @@ struct OpeningBraceConfiguration: SeverityBasedRuleConfiguration { | |||
| private(set) var severityConfiguration = SeverityConfiguration<Parent>(.warning) | |||
| @ConfigurationElement(key: "allow_multiline_func") | |||
| private(set) var allowMultilineFunc = false | |||
| @ConfigurationElement(key: "allow_multiline_statement") | |||
| private(set) var allowMultilineStatement = true | |||
There was a problem hiding this comment.
The name does not reflect its meaning. First off, it's not "allow" but "ignore" because the position of the opening brace isn't of any relevance once the statement is "multiline". Secondly, the rule doesn't care if the whole statement is multiline, but only if its conditions is.
In my opinion, the name of the existing similar option allowMultilineFunc is already misleading. This can be fixed in another PR, though.
leonardosrodrigues0
force-pushed
the
opening-brace-multiline-statements
branch
from
c1a6b85
to
c970554
Compare
leonardosrodrigues0
force-pushed
the
opening-brace-multiline-statements
branch
from
c970554
to
8876cd2
Compare
From the issue and my experience, the main focus would be guard
let child = parent.children.first,
let grandchild = child.children.first,
let greatGrandchild = grandchild.children.first
else {
// Some code
}I did, however, take a look at the OSS differences when we ignore the rule for multiline "predecessors of the body" for all statements affected by the rule. I checked all differences and it seems that this style is frequently adopted in type declarations (and extensions), as in: extension RawRepresentable
where
Self: AtomicOptionalRepresentable,
RawValue: AtomicOptionalRepresentable
{
// Some code
}Considering this, I think that the new direction should be to let the option ignore all statements that are "multiline before the opening brace" (except for single keywords like Changing the existing option name and make it affect all statements would be a breaking change, and I don't have experience on how these are made in this project. Would we still support Additionally, the new option name could be something like |
|
Thank you for the write-up! So the topic applies to functions/initializers (already supported), types and statements (implemented by this PR). I think that an existing option shouldn't suddenly support much more than it was intended for. But we may rename Statements like Types might be yet another category. We could merge them with the control-flow statements from the previous group (as both groups will be newly introduced). However, this doesn't really match, which would already be problematic when looking for an option name. Talking about names: How about |
|
I like the functions/initializers, types and statements separation and the name for the configuration options, I'll implement that.
How would we do that? I found this example being removed in #5107, should we mark the option as deprecated like this? mutating func apply(configuration: Any) throws {
// ...
} else if let statementLevelConfiguration = configurationDict["statement_level"] {
queuedPrintError(
"""
warning: 'statement_level' has been renamed to 'function_level' and will be completely removed \
in a future release.
"""
)
try functionLevel.apply(configuration: statementLevelConfiguration)
}
// ...
} |
The new macro approach doesn't support deprecation warnings well yet. I'm working on it though. For our PR, just leave the existing option as is for now. I'll take care of the rename once I found a good solution. |
I've added a way to mark options as deprecated in #5540. Just in case you want to handle the rename here as well ... 😉 |
In case of multiline
ifandwhilestatements, many adopt the style of putting the opening brace in a new line. This change add a new option to the rule to allow that style.Closes #3720