-
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
Add option for opening_brace
rule not to trigger with multiline statements
#5521
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c1a6b85
to
c970554
Compare
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 ... 😉 |
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.
Hello everyone! I've never contributed here, but my team would also like to see these changes. Great idea! While looking at the proposed solution, I added a couple of suggestions / questions. Thank you @leonardosrodrigues0 for your efforts!
@@ -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: "ignore_multiline_condition_statements") | |||
private(set) var ignoreMultilineConditionStatement = true |
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.
In consideration of naming...
Based on the perceived intentions, the following applies (not exhaustively) to "statement:"
- statement -> loop-statement (e.g., for-in, while, repeat-while)
- statement -> branch-statement (e.g. if, guard, switch)
The "multiline" term applies to the conditions of loop (while) and branch (if, and guard).
Given the above, the following name seems most applicable (and was the initial recommendation):
ignoreMultilineStatementConditions
As for guard, the opening brace immediately follows the else, not the condition list, and thus should be excluded, IMO.
if isIgnoredMultilineConditionStatement(body, keywordToken: node.enumKeyword) { | ||
return | ||
} |
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.
An enum definition could have multiline protocol adherence, but I"m not aware of a condition list. So, is this needed?
The same question applies to all other applications of this logic that do not have an associated condition list that precedes the opening brace.
In case of multiline
if
andwhile
statements, 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