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

[FINE] convert Vm#miq_provision_template to has_one #17309

Merged
merged 3 commits into from Apr 18, 2018

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Apr 17, 2018

imtayadeway and others added 3 commits April 17, 2018 14:47
When `Field` was extracted it was attractive to let it be a middleman
between `MiqExpression` and the Arel library since everything goes
through `Field#arel_attribute`. While this started out as
straightforward delegation, more supporting code was required to
bridge more complex expressions such as `CONTAINS`, or things that
require extra arguments like `matches`, etc..

I think that consequently `Field` (and `Tag`) have taken too much on
in terms of responsibility. It also introduced a cohesion problem in
that not all fields are concerned with columns in the database.

While this change might be a step back for `MiqExpression`, I think it
greatly improves `Field`. I think there are better patterns out there
for better separating responsibilities here, but that can be left to a
future revision.
convert Vm#miq_provision_template to has_one

(cherry picked from commit eef19e0)
@miq-bot
Copy link
Member

miq-bot commented Apr 17, 2018

Checked commits kbrock/manageiq@158b35b~...d68a133 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 6 offenses detected

lib/miq_expression.rb

lib/miq_expression/field.rb

  • ❗ - Line 80, Col 5 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

lib/miq_expression/where_extraction_visitor.rb

@simaishi simaishi requested a review from gtanzillo April 17, 2018 19:37
@simaishi simaishi changed the title [fine] convert Vm#miq_provision_template to has_one [FINE] convert Vm#miq_provision_template to has_one Apr 17, 2018
@simaishi simaishi removed the fine/yes label Apr 17, 2018
@kbrock kbrock requested review from imtayadeway and gtanzillo and removed request for gtanzillo April 17, 2018 20:20
@JPrause
Copy link
Member

JPrause commented Apr 18, 2018

@miq-bot add_label blocker

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

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

LGTM

@simaishi simaishi merged commit 7702cc8 into ManageIQ:fine Apr 18, 2018
@simaishi simaishi added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 18, 2018
@kbrock kbrock deleted the vm_template_name_f branch April 18, 2018 15:38
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

7 participants