Make Gitlab::Ci::Config::Entry::Node#inspect human readable
For now Gitlab::Ci::Config::Entry::Node#inspect is not readable therefore making debugging harder. In gitlab-ce!9666 a simple version is implemented as follow:
def inspect
val = leaf? ? config : descendants
unspecified = specified? ? '' : '(unspecified) '
"#<#{self.class.name} #{unspecified}{#{key}: #{val.inspect}}>"
end
This would show something like:
#<Job {job0: [#<Stage {stage: "test"}>]}>
However this is misleading, because it should actually show:
#<Job {job0: [#<Stage (unspecified) {stage: "test"}>]}>
The reason why it's always showing specified instead of properly showing unspecified, is because Gitlab::Ci::Config::Entry::Unspecified is a wrapper around Stage. Therefore whenever Unspecified#inspect is called, it's delegating to Stage#inspect, effectively negate the effect of the wrapper.
Originally, I solved this by removing Unspecified, and added an attribute to the Node to indicate it's specified or not. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9666#note_24608178
However we don't yet have consensus about what we should do here, and it's unrelated to that particular merge request therefore I just reverted it.
Here's what I think we could do:
- Remove
Unspecifiedand add an attribute toNodeas I originally did. This was chosen not because I think this was the best approach, just because that's what came to my mind first and I wanted to complete the merge request sooner than later. This is no longer a concern. The downside of doing so is that then we're manipulating states, which is bad. - Also implement
inspectforUnspecified. This wayspecified?could be properly detected. The downside of doing so is that we'll need to implementinspecttwice, identically. We could reduce repetition by using a module and include (or prepend) it, but this doesn't feel right to me either.
Can we find a better solution here?
/cc @grzesiek