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

Partitioner: add menu actions #1142

Merged

Conversation

joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Sep 23, 2020

This is part of the on-going process to reorganize the Partitioner UI into the branch partitioner-ui-02.

This PR makes the Device and Add menus completely functional by calling the proper action for every item of the menus.

@coveralls
Copy link

coveralls commented Sep 23, 2020

Coverage Status

Coverage increased (+0.3%) to 97.346% when pulling 94b9ecd on joseivanlopez:device_menu_actions into ffc7345 on yast:partitioner-ui-02.

@joseivanlopez joseivanlopez changed the title Partitioner: add Device menu actions Partitioner: add menu actions Sep 23, 2020
end

private

# @see Base
# rubocop:disable Metrics/CyclomaticComplexity
# Is this a complex method actually?
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it likely could be substituted by a hash and/or a little bit of metaprogramming (generating the name of the method and then using send).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure. In fact, I did it with metaprogramming, but the result is more difficult to read/understand. In this kind of things, I am not able to understand why a simple case has high complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if you tried and it looked worse, then I guess this is fine. I didn't actually tried, I was just assuming it could look good. But experience beats assumptions. :-)

Copy link
Member

Choose a reason for hiding this comment

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

well, in general case is usually hidden class separation. Here it is done for actions. So if all actions has simple format of class.new or class.new(device). you can use something like MENU_ADD_MAPPING[event].new(device) So hash based factory.

Copy link
Member

Choose a reason for hiding this comment

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

and btw if item is proper object, code can look like items.find { |i| i.id == event }.action and also above it can create list of items from object like @items ||= [add_md.item, add_vg.item, ..]

@@ -75,12 +81,34 @@ def disabled_for_device
end

# @see Base
# rubocop:disable Metrics/CyclomaticComplexity
# Is this a complex method actually?
Copy link
Contributor

Choose a reason for hiding this comment

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

Like before, this could be probably improved with a bit of send or similar.

Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM

@joseivanlopez joseivanlopez merged commit 05769b7 into yast:partitioner-ui-02 Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants