pkg/pod: flatten the pod state if-ladders #3404
Conversation
| } | ||
| stdout.Print(string(result)) | ||
| case "json-pretty": | ||
| result, err := json.MarshalIndent(pods, "", "\t") | ||
| if err != nil { | ||
| stderr.PrintE("error marshaling the pods", err) | ||
| return 254 | ||
| stderr.FatalE("error marshaling the pods", err) |
There was a problem hiding this comment.
I guess the changes in this file are spurious and belong to #3403.
| {dir: p.preparedPath(), impliedStates: []*bool{&p.isPrepared}}, | ||
| // For run, assume exited until the lock is tested | ||
| {dir: p.runPath(), impliedStates: []*bool{&p.isExited}}, | ||
| {dir: p.garbagePath(), impliedStates: []*bool{&p.isGarbage}}, |
There was a problem hiding this comment.
There seems to be a re-ordering happening here between garbage and exitedgarbage. I'm not sure if it is on purpose and if the result is equivalent, but reading the original code I would have expected the last two here to be swapped.
There was a problem hiding this comment.
I'll swap them back, that wasn't intentional.
However, since the valid transitions are run -> exited garbage and prepare -> garbage, the order between those two shouldn't matter, so long as they're after their respective prior states.
| {dir: p.runPath(), impliedStates: []*bool{&p.isExited}}, | ||
| {dir: p.garbagePath(), impliedStates: []*bool{&p.isGarbage}}, | ||
| // Exited garbage implies exited | ||
| {dir: p.exitedGarbagePath(), impliedStates: []*bool{&p.isExitedGarbage, &p.isExited}}, |
| // dirStates is a list of directories -> state that directory existing | ||
| // implies. | ||
| // Its order matches the order states occur. | ||
| dirStates := []struct { |
There was a problem hiding this comment.
Thanks, this is a much nicer way of expressing this logic.
What about each row having two sets of states - the isLocked and isUnlocked states? That would make the logic a bit further down simpler to grok.
There was a problem hiding this comment.
I gave it a try, but I'm not sure it actually ends up being that much more readable with only a trivial amount more work. here's the wip exploration, but the end result neither worked nor looked that much prettier.
I'd rather take this and stack on further refactoring as separate PRs so long as we all agree this specific change is good by itself.
|
There's been enough review comments, I just wanted to give a general 👍 for this :-) |
euank
force-pushed
the
flatten-a-ladder
branch
from
773deb0
to
220fb12
Compare
|
Addressed @lucab's comments and squashed |
lucab
changed the title
I think this is more readable.
This is just a refactor, as well as adding a few tests so the refactor was more likely to be correct.