Remove value2tag for count field in MiqExpression

[lpichler]

• it is not needed to have value2tag here
  method is only calling MiqExpression::CountField.parse in this situation
• updating specs: correct format is
  model.assocation (MiqExpression::CountField::REGEX)

🥅 Sub goal : Remove value2tag- code inside of this method is basically calling other method on related instance of MiqExpression::Target and code after each calling of value2tag looks related to MiqExpression::Target (or childs) - so it could be encapsulated

⚽️ Goal: Remove useless code, simplification of MiqExpresion and finding code/patterns which could be live by MiqExpression::Target (childs) - moving responsibility to MiqExpression::Target (childs)

@miq-bot add_label refactoring, reporting

@miq-bot assign @kbrock

[kbrock]

Thanks for finding this.

With this class, I think it works better if we walk before running here.
I like the way Arel works in Rails. They generate an AST (which we do) and use a visitor pattern to generate the sql rather than have nodes with a to_sql method.

Can we try making this more verbose before we dry it up?

Introduce more calls to parse_field_or_tag - hopefully once per method (or passing a Field object around). Lets drop our use of get_col_type and value2tag - like you suggested. This will possibly make it more verbose at first.

I just want to trying to get all the code looking as similar as possible first.
Lets see if we can find duplicate code that no longer needs case statements.

From here, I believe the code will become more approachable.
We were able to get to_arel close to 1 line per operation. Maybe we can do the same here.

Some examples (I very possibly flubbed these - please double check my work)

    elsif ops["count"]
      target = parse_field_or_tag(ops["count"])
      fld = "<count ref=#{target.model.to_s.downcase}>#{target.tag_path_with}</count>"
      [fld, quote(ops["value"], field.col_type)]
    elsif ops["regkey"]

        field = parse_field_or_tag(ops["field"])
        case context_type
        when "hash"
          fld = "<value type=#{field.col_type}>#{field.to_s}</value>"
        else
          fld = "<value ref=#{field.model.to_s.downcase}, type=#{field.col_type}>#{field.tag_path_with(val)}</value>"
        end
        if ["like", "not like", "starts with", "ends with", "includes", "regular expression matches", "regular expression does not match"].include?(operator)
          [fld, ops["value"]]
        else
          [fld, quote(ops["value"], field.col_type)]
        end

  def self.quote(val, typ)
    if (target = parse_field_or_tag(val) rescue nil)
      return "<value ref=#{target.model.to_s.downcase}, type=#{target.col_type}>#{value}</value>"
    end

[kbrock]

1. I remember seeing both forms Vm-snapshots and Vm.snapshots. Could you explain the difference?
2. I didn't think Expression::Field supported an association

[lpichler]

I remember seeing both forms Vm-snapshots and Vm.snapshots. Could you explain the difference?

Vm.snapshots - this is relation
Vm-snapshots - this is column (MiqExpression::CountField::REGEX)

I didn't think Expression::Field supported an association
yes it is for CountField to filter according to count of records in relation.

[Fryguy]

🥅 🥅 Sub-sub goal : Remove value2tag because one should never use 2 to mean "to" nor 4 to mean "for" 😆

[jrafanie]

🥅 🥅 Sub-sub goal : Remove value2tag because one should never use 2 to mean "to" nor 4 to mean "for" 😆

-1 NACK

[lpichler]

Some examples (I very possibly flubbed these - please double check my work)

@kbrock I agree with you - I added it in last commit. (I left the second commit to keep this idea in history If we will need it)

I will continue with removing get_col_type and rest of value2tag in other PRs (regard to your suggestions)

[miq-bot]

Checked commits lpichler/manageiq@46af818~...c080a86 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰