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

GO cleanup #2635

Merged
merged 8 commits into from
Nov 6, 2017
Merged

GO cleanup #2635

merged 8 commits into from
Nov 6, 2017

Conversation

martinpovolny
Copy link
Member

@martinpovolny martinpovolny commented Nov 6, 2017

A quick round of cleanups for the new GO code.

TODO: Other issues that I have noticed:

  • NEW instance variables are being used in buttons
app/helpers/application_helper/button/generic_object_definition_button_delete.rb:    @record.kind_of?(GenericObjectDefinition) && @display != 'generic_objects' && !@actions_node
app/helpers/application_helper/button/generic_object_definition_button_edit.rb:    !(@display == 'generic_objects' || [email protected]_of?(GenericObjectDefinition) || @actions_node)
  • app/views/generic_object_definition/_show_list.html.haml -- a view that basically just contains a branching logic that should be in a controller

I think that this needs to go to the release, because we are going to see more work and fixes in this new area and we don't want to do the work twice.

Methods that deal with a node should get the node as an argument, not
from 2 different side channels. This way it's easier to follow data flow
in the code and write tests.
This actually saves a lot of repetition of string operations and provides
better readability.
ExplorePresenter expects a hash not a string in :activate_node. So this has no
positive effect.
node_info is called only from one place and x_node is set there.
Removing unneeded condition, pass data as an argument.
@martinpovolny martinpovolny removed the wip label Nov 6, 2017
@martinpovolny martinpovolny changed the title [WIP] GO cleanup GO cleanup Nov 6, 2017
@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2017

Checked commits martinpovolny/manageiq-ui-classic@af249e3~...7e515c6 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@himdel
Copy link
Contributor

himdel commented Nov 6, 2017

@jhernand The card is private, but judging from the title, I'm guessing this is not related to generic object definitions?

@jhernand
Copy link
Contributor

jhernand commented Nov 6, 2017

@himdel yes, just bad clicking in the Trello UI, my fault. I undid it.

@himdel
Copy link
Contributor

himdel commented Nov 6, 2017

LGTM, the only ui breakage I see (not being able to create a custom button) is present in master as well (and hopefully fixed in #2569) 👍
Merging.

@himdel himdel merged commit a0a1c1a into ManageIQ:master Nov 6, 2017
@himdel himdel added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 6, 2017
AparnaKarve added a commit to AparnaKarve/manageiq-ui-classic that referenced this pull request Nov 6, 2017
simaishi pushed a commit that referenced this pull request Nov 7, 2017
GO cleanup
(cherry picked from commit a0a1c1a)
@simaishi
Copy link
Contributor

simaishi commented Nov 7, 2017

Gaprindashvili backport details:

$ git log -1
commit 4c33d0f491e5c5879534638a8a0e9a8602faa3ee
Author: Martin Hradil <[email protected]>
Date:   Mon Nov 6 14:06:41 2017 +0000

    Merge pull request #2635 from martinpovolny/go_cleanup
    
    GO cleanup
    (cherry picked from commit a0a1c1a709e9e051a5c7353ebb6d1cdd01e70718)

AparnaKarve added a commit to AparnaKarve/manageiq-ui-classic that referenced this pull request Nov 8, 2017
AparnaKarve added a commit to AparnaKarve/manageiq-ui-classic that referenced this pull request Nov 9, 2017
AparnaKarve added a commit to AparnaKarve/manageiq-ui-classic that referenced this pull request Nov 13, 2017
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.

7 participants