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

Remove the second (type) argument from TreeBuilder classes #5495

Merged
merged 1 commit into from
May 2, 2019

Conversation

skateman
Copy link
Member

@skateman skateman commented Apr 25, 2019

Now the type and name arguments are completely redundant and the type is not used anywhere so it is time to remove it.

There are some tree initialization calls where the two arguments were swapped in order. This might cause some problems with x_tree and x_active_tree in the area as the name of the tree might not had the _tree suffix. Here are those:

  • TreeBuilderSections
  • TreeBuilderProtect
  • TreeBuilderClusters
  • TreeBuilderDatastores
  • TreeBuilderSmartproxyAffinity

Also I might have missed some cases where metaprogramming comes into the game so would be nice to go through some of the non-explorer trees a little.

@miq-bot add_label trees, cleanup, hammer/no
@miq-bot add_reviewer @ZitaNemeckova

@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2019

Checked commit skateman@093709d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
118 files checked, 0 offenses detected
Everything looks fine. ⭐

@skateman skateman changed the title [WIP] Remove the second (type) argument from TreeBuilder classes Remove the second (type) argument from TreeBuilder classes Apr 26, 2019
@miq-bot miq-bot removed the wip label Apr 26, 2019
@martinpovolny
Copy link
Member

It's cool someone (you) is moving forward with the tree.

Copy link
Member

@martinpovolny martinpovolny left a comment

Choose a reason for hiding this comment

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

The code looks good. But I have not tested this in the UI.

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Tested, LGTM 👍

@martinpovolny martinpovolny merged commit 6b9f653 into ManageIQ:master May 2, 2019
@martinpovolny martinpovolny self-assigned this May 2, 2019
@martinpovolny martinpovolny added this to the Sprint 111 Ending May 13, 2019 milestone May 2, 2019
@skateman
Copy link
Member Author

skateman commented May 2, 2019

@miq-bot add_label changelog/yes

@skateman skateman deleted the drop-tree-type branch May 2, 2019 13:49
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 8, 2019
Fixed 2 issues
- Fixed `wrong number of arguments (given 4, expected 3)` error, removed 'type' argument that was being passed in to tree builder code, was introduced by ManageIQ#5495
- Fixed `comparison of Integer with String failed` error for `@assign[:new][:objects].sort!`, changed to push integer into objects array to fix comparison error on the sort when selecting nodes in the tree in 'Selections' box.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1707871
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1707886
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 8, 2019
Fixed 2 issues
- Fixed `wrong number of arguments (given 4, expected 3)` error, removed 'type' argument that was being passed in to tree builder code, was introduced by ManageIQ#5495
- Fixed `comparison of Integer with String failed` error for `@assign[:new][:objects].sort!`, changed to push integer into objects array to fix comparison error on the sort when selecting nodes in the tree in 'Selections' box.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1707871
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1707886
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.

4 participants