-
Notifications
You must be signed in to change notification settings - Fork 440
[NFC] Simplify some value bindings #1406
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
[NFC] Simplify some value bindings #1406
Conversation
@swift-ci please test |
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 understand that code style is always a little subjective but I disagree with you here. I think at the old version is more readable because it has one fewer nesting level. Which part of the old implementation did you find hard to read? Maybe we can find some style that works for both of us.
I suppose it wasn't hard to read. It just felt a little weird to me that
What if the 3 patterns are indented to align vertically? That probably would improve the "from a single glance" aspect imo. Another reason for this PR is that I wanted to land in some small preparations for implementing switch self.at(...) {
case .some(patternStart, handle):
if context == .matching {
addDiagnostics(...)
}
switch patternStart {...}
case .nil:
...
} than this: switch self.at(...) {
case .varKeyword, .letKeyword, .inoutKeyword:
if context == .matching {
addDiagnostics(...)
}
...
case .isKeyword:
if context == .matching {
addDiagnostics(...)
}
...
case .nil:
...
} But I might be jumping way too ahead here. |
I don’t really have a problem with binding
I agree that that would help. I’m wondering whether we should fix that in
I don’t fully understand what you’re trying to say here. Also where are these examples coming from? |
We've resisted #2 in the past in favor of having a uniform indentation step throughout. This isn't really much different than the issue you have with
where breaking after the The least invasive change to make would be to change this EDIT: Actually, it's more complicated than that because of the grouping around the Longer-term, it might be worth revisiting the rigid uniform indentation step, but I don't want to do that quite yet since it would have much broader implications to how the formatting algorithm works. |
I think I can explain my rationale better when I have the PR for Though I admit that the more I think about it, the less merit I see in pushing for this PR. |
OK, let’s just put this PR on hold then for now and re-evaluate it when you’ve got your other PR up. |
It turned out I misunderstood how diagnostics is produced, so my second reason for this PR doesn't stand. I don't really want to change the code for minor aesthetic reasons alone, so I'm closing this PR. Thanks for the review! |
Aesthetic changes are always welcome, even if they are minor, so don't feel like you shouldn't create PRs like this in the future. We just need to agree that the change is for the better 😉 |
This makes the bindings a little easier to read imo.
Also this might pave a little way for implementing the pitched
is case
, if we decide to banfor being too silly.