-
Notifications
You must be signed in to change notification settings - Fork 886
pkg/pod: flatten the pod state if-ladders #3404
Conversation
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.
One concern related to a logic re-ordering which I'm not sure about.
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}}, |
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.
Same comment as above.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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.
sgtm
There's been enough review comments, I just wanted to give a general 👍 for this :-) |
773deb0
to
220fb12
Compare
Addressed @lucab's comments and squashed |
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.