Re-enable three swift-format rules. #2869
Conversation
allevato
requested review from
DougGregor,
ahoppen and
bnbarham
as code owners
There was a problem hiding this comment.
UseSynthesizedInitializer: Looks good to meNoBlockComments: I highlighted the cases where I think that block comments are actually the right choice. I’m mildly leaning towards keeping that rule off. Pretty impartial to migrating the block comments that I didn’t comment on to line comments.UseLetInEveryBoundCaseVariable: I really don’t have an opinion here, happy with it on or off
| node = operatorTable.foldAll(node, errorHandler: { _ in /*ignore*/ }) | ||
| node = operatorTable.foldAll(node, errorHandler: { _ in }) // ignore |
There was a problem hiding this comment.
I think this is more readable with the block comment, same for the other ignore comments.
There was a problem hiding this comment.
I totally agree. I think in most cases if the comment block is placed between nodes, it shouldn’t be moved to the end?
| for suffix in [ | ||
| "V", // struct | ||
| "O", // enum | ||
| "C", // class | ||
| ] { |
There was a problem hiding this comment.
Similarly here, I think disallowing block comments decreases readability.
| count: buffer.count + /* for NULL */ 1 | ||
| count: buffer.count + 1 // +1 for NULL |
There was a problem hiding this comment.
And here I think the block comment also makes sense.
| node = operatorTable.foldAll(node, errorHandler: { _ in /*ignore*/ }) | ||
| node = operatorTable.foldAll(node, errorHandler: { _ in }) // ignore |
There was a problem hiding this comment.
I totally agree. I think in most cases if the comment block is placed between nodes, it shouldn’t be moved to the end?
| "UseLetInEveryBoundCaseVariable": false, | ||
| "UseSynthesizedInitializer": false | ||
| "UseLetInEveryBoundCaseVariable": true, | ||
| "UseSynthesizedInitializer": true |
There was a problem hiding this comment.
I’ve noticed that @ahoppen mentioned not having a strong opinion here, so I’d like to share my perspective. In my projects, when a case in a closure has multiple parameters, and I want to designate one parameter as a variable (var) and the others as constants (let), applying this rule makes sense since you can see what is mutable and what's not–don't know if this statment makes sense to u XD hope so.
Code example:
enum Foo {
case bar(name: String: age: Int)
}
let foo = Foo.bar(name: "Tom", age: 22)
if case .bar(let name, var age) = foo {
age += 1
// do sth with increased age and not mutated name?
}There was a problem hiding this comment.
This is one of the reasons I prefer UseLetInEveryBoundCaseVariable as well. Wrapping the whole pattern with let/var forces you into an all-or-nothing situation with the bindings. Once we have other kinds of bindings like borrowing, inout, etc., I think it'll be even more common for those to differ within a payload.
The other reason for this rule is that having the binding specifier immediately adjacent to the binding makes it easier to see what the intended outcome is, and avoids mistakes like this:
let age = 20
if case let .bar(name, age) = foo { ... }
// oops I wanted to bind `name` and match `age`, not create a
// new `age` binding
if case .bar(let name, age) = foo { ... }
// much clearer about which one is a new binding and which
// is an expression to be matched
allevato
force-pushed
the
enable-some-rules
branch
from
8335b63
to
d44de72
Compare
After making the actual changes, I tend to agree for these cases. I've dropped that commit from the PR. I think it reveals a potential improvement to the |
| "UseLetInEveryBoundCaseVariable": true, | ||
| "UseSynthesizedInitializer": true |
There was a problem hiding this comment.
These are true by default, right? So we should be able to remove them from .swift-format to make it shorter.
There was a problem hiding this comment.
That's what I thought as well, but I just noticed that if I remove the line entirely and insert a violation, it isn't flagged.
Oof. I think this is a consequence of the decoding logic; if the rules dictionary is present in the JSON, its value is used verbatim. Only if it isn't present at all do we use the default dictionary. Then, when we check if a rule should be applied at a node, if the rule name isn't in the dictionary, it's always false.
We should fix this by always taking the default rule settings and then merging what we read from the JSON into that. That would ensure that the in-memory dictionary always contains all rules. That might reveal some other places in swift-syntax/swift-format where the rules aren't flagging violations, though.
So my thinking is we merge this as-is ("OrderedImports" was already this way) and then fix that for the next release. WDYT?
There was a problem hiding this comment.
Sounds good to me. But yes, this seems like a swift-format bug to me to not merge the dictionaries that’s worth fixing. If you don’t have time to look at that now, could you file an issue for it?
|
@swift-ci Please test |
|
@swift-ci please test macOS platform |
This PR re-enables the following three rules, with the associated fixes:
UseSynthesizedInitializerNoBlockCommentsUseLetInEveryBoundCaseVariable