SwiftLintBot
commented
3 months ago Generated by :no_entry_sign: Danger
Open leonardosrodrigues0 opened 3 months ago
SwiftLintBot
commented
3 months ago Generated by :no_entry_sign: Danger
leonardosrodrigues0
commented
3 months ago @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.
leonardosrodrigues0
commented
2 months ago I wonder why this new option does not apply likewise to
guardandfor(and maybe other statements). Any reason for that?
From the issue and my experience, the main focus would be if, while and guard, but guard has the else keyword that is used in the same line as the opening brace in this type of cases:
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 do and defer). And if we go this way, I don't see a reason why we would leave functions and initializers (that are already ignored by the allowMultilineFunc option) with an independent option of all other statements.
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 allowMultilineFunc but deprecate it?
Additionally, the new option name could be something like ignoreMultilineBracePredecessors, ignoreMultilineOpeningBracePredecessors or ignoreMultilineBodyPredecessors. I'm not sure if "predecessors" fits well here, but it is the best description I could create. On the other hand, ignoreMultilineStatements would be simpler but imprecise, as you pointed out.
SimplyDanny
commented
2 months ago 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 allow_multiline_func to something better matching while still supporting the current name for the time being.
Statements like if, while, guard and for should be associated with a separate option.
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 ignoreMultilineFunctionSignatures, ignoreMultilineStatementConditions and ignoreMultilineTypeHeaders?
leonardosrodrigues0
commented
2 months ago I like the functions/initializers, types and statements separation and the name for the configuration options, I'll implement that.
But we may rename
allow_multiline_functo something better matching while still supporting the current name for the time being.
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)
}
// ...
}How would we do that? I found this example being removed in https://github.com/realm/SwiftLint/pull/5107, should we mark the option as deprecated like this?
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.
SimplyDanny
commented
2 months ago How would we do that? I found this example being removed in #5107, should we mark the option as deprecated like this?
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