chore: test what gofumpt looks like #723
Conversation
| SpawnTime: time.Date(2016, 0o4, 21, 1, 2, 3, 0, time.UTC), | ||
| ReadyTime: time.Date(2016, 0o4, 21, 1, 2, 4, 0, time.UTC), | ||
| }}, | ||
|
|
||
| SpawnTime: time.Date(2016, 04, 21, 1, 2, 3, 0, time.UTC), | ||
| ReadyTime: time.Date(2016, 04, 21, 1, 2, 4, 0, time.UTC), | ||
| SpawnTime: time.Date(2016, 0o4, 21, 1, 2, 3, 0, time.UTC), | ||
| ReadyTime: time.Date(2016, 0o4, 21, 1, 2, 4, 0, time.UTC), |
There was a problem hiding this comment.
I'd rather see 4 than 0o4 here.
At the same time, the hidden octal 04 was kind nasty, any change is better than no change.
| }{{ // API source: Unix Domain Socket | ||
| // User: nil | ||
| apiSource: daemon.TransportTypeUnixSocket, | ||
| user: nil, | ||
| openCheckErr: nil, | ||
| adminCheckErr: errUnauthorized, | ||
| userCheckErr: errUnauthorized, | ||
| metricsCheckErr: errUnauthorized, | ||
| pairingCheckErr: errUnauthorized, | ||
| }, { | ||
| // User access: UntrustedAccess | ||
| apiSource: daemon.TransportTypeUnixSocket, |
There was a problem hiding this comment.
I'm happy with the change, though:
- the comments look a bit inconsistent now, how come the "API" comment shares a line with the opening curly, but "Untrusted" doesn't?
- the first comment was meant to apply to the block, and now it doesn't... perhaps the this code needs to be refactored so that variable names or keys are used instead of comments
Something like:
type accessTest struct {...}
var (
untrustedUser = &daemon.UserState{Access: state.UntrustedAccess}
metricsUser = &daemon.UserState{Access: state.MetricsAccess}
...
var unixSocketTests = []accessTest{
// User: nil
{
apiSource: daemon.TransportTypeUnixSocket,
user: nil,
...
},{
...
}
}
...
func ... TestAccess (...) {
tests := nil
tests = append(tests, unixSocketTests...)
tests = append(tests, httpTests...)
...
}| }, | ||
| { |
There was a problem hiding this comment.
This change seems to be the opposite of the test change above?
There was a problem hiding this comment.
I think that this is one of the spots that Ben mentioned in the PR description:
The one change I wasn't happy about is the change from }, { between struct literals, to },\n{. But it seems it allows the one-line format sometimes, so I'm not sure if we can tweak that behaviour, or if it matters enough to worry about.
UPDATE: I discovered gofumpt is fine with this, we just had some inconsistent comments between struct entries. See these commits for the fix (we'll need to fix manually in a few other places too).
See the // Some basics. comment above. That comment doesn't appear to add significant value and could probably be removed.
| var ( | ||
| timeNow = time.Now | ||
| ) | ||
| var timeNow = time.Now |
There was a problem hiding this comment.
Looks like this only so that it can be mocked out in tests.
Is this a Go idiom, or does this dependency injection point deserve a comment?
| expected: []*timeutil.Schedule{ | ||
| { | ||
| WeekSpans: []timeutil.WeekSpan{ | ||
| {Start: timeutil.Week{Weekday: time.Monday}, End: timeutil.Week{Weekday: time.Monday}}, | ||
| }, | ||
| }, { | ||
| WeekSpans: []timeutil.WeekSpan{ | ||
| {Start: timeutil.Week{Weekday: time.Wednesday}, End: timeutil.Week{Weekday: time.Wednesday}}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
perhaps a bit too verbose, but these are tests, no biggie.
| layers: []string{ | ||
| ` |
There was a problem hiding this comment.
Does that technically change the leading whitespace in the multiline string literal?
There was a problem hiding this comment.
It doesn't look like it. The same number of newlines exist within the backticks, a newline is just added before.
3 participants
DRAFT: do not merge without further discussion. This is the result of running
gofumpt -l -w .on the Pebble codebase, to evaluate whether we want to use it.The
time.Datearguments, which gofumpt (I think properly) changes from04to0o4, should be changes to just4to avoid this. We clearly intend decimal here -- there will be some manual edits required to fix these. But the other octal changes like0o777look fine.Most of the other changes look good.
The one change I wasn't happy about is the change from
}, {between struct literals, to},\n{. But it seems it allows the one-line format sometimes, so I'm not sure if we can tweak that behaviour, or if it matters enough to worry about.UPDATE: I discovered gofumpt is fine with this, we just had some inconsistent comments between struct entries. See these commits for the fix (we'll need to fix manually in a few other places too).