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: ``` ruby 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 `Unspecified` and add an attribute to `Node` as 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 `inspect` for `Unspecified`. This way `specified?` could be properly detected. The downside of doing so is that we'll need to implement `inspect` twice, 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
issue