Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call TreeBuilder#override on TreeNode objects instead of hashes #6220

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

skateman
Copy link
Member

@skateman skateman commented Sep 20, 2019

There's a controversial TreeBuilder#override method that is intended to change nodes after they are being built. This method has been called from TreeBuilder#x_build_single_node on individual hashes that come out from TreeNode::Node#to_h.

The final result of the x_build_single_node has been not changed, so the method still produces the same output. However, the override method is now being called on the TreeNode::Node object directly and it is being converted to a hash AFTER this. This allows us to have a cleaner dataflow and eventually the hash conversion can be pushed up to the highest level, leaving us with a pure object tree.

As the override happened after the conversion, the resulted node had to comply with the JSON specification of the tree. This forced us to reimplement features from the TreeNode::Node#to_h, such as lazy-initializing the state hash appending the extra no-cursor class to non-selectable nodes. These safeguards are no longer necessary and they have been removed.

@miq-bot add_label refactoring, trees, ivanchuk/no
@miq-bot add_reviewer @martinpovolny
@miq-bot add_reviewer @romanblanco
@miq-bot add_reviewer @mzazrivec
@miq-bot add_reviewer @ZitaNemeckova

@skateman
Copy link
Member Author

CI will stay red until #6221 is merged.

@skateman skateman force-pushed the treenode-override-obj branch from c78bd75 to 35425f5 Compare September 20, 2019 15:13
@miq-bot
Copy link
Member

miq-bot commented Sep 20, 2019

Checked commit skateman@35425f5 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
19 files checked, 0 offenses detected
Everything looks fine. 🍪

@@ -1,6 +1,6 @@
class TreeBuilderAutomateEntrypoint < TreeBuilderAutomateCatalog
def override(node, _object)
node.delete(:selectable)
node.selectable = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false would be better? (to be consistent with other places)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be, but it is not what we want. Setting it to false means that the node is not selectable, setting it to nil means that the value is not set. And if you check the original line, that's exactly what I need.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{:foo => false, :bar => nil}.compact
=> {:foo=>false}

@martinpovolny
Copy link
Member

Looks good!

@martinpovolny martinpovolny merged commit 4862957 into ManageIQ:master Sep 24, 2019
@martinpovolny martinpovolny self-assigned this Sep 24, 2019
@martinpovolny martinpovolny added this to the Sprint 121 Ending Sep 30, 2019 milestone Sep 24, 2019
@skateman skateman deleted the treenode-override-obj branch September 24, 2019 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants