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

Fixes #21634 - Allows modification of output fields #275

Merged
merged 1 commit into from Nov 30, 2018

Conversation

Projects
None yet
4 participants
@ofedoren
Copy link
Member

ofedoren commented May 31, 2018

No description provided.

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Jun 1, 2018

Thanks @ofedoren! It looks good at a first glance. Would you mind adding some docs with examples how to change a commands output from a plugin? It would make testing easier plus we can refer to them in future.

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch 2 times, most recently from a7a8eda to a3f46d1 Jun 4, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Jun 4, 2018

@tstrachota updated. I did some changes comparing to the last version of this commit and added some docs.

@tstrachota
Copy link
Member

tstrachota left a comment

Thank you for adding the docs. It was much easier to understand now.

Some changes are needed, I mentioned them all in the code.

There are some features that I'm missing, quite important ones in my opinion:

  1. finding fields, to make a modification on them. Eg.:
d.find_fields(:id => :name)[0].description = _('Some better description')
  1. inserting fields at a given position. Eg.:
d.insert_fields(:after_id => :operating_system) do # or before_id
  label _('Ansible') do
    # ...
  end
end
def at(path = [])
return self if path.empty?
field = expand_path(@fields, path.dup)
return nil if field.nil? || !field.respond_to?(:output_definition)

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

This should raise an error rather than return nil. In 99% of cases at will be directly followed by calling some other methods on the output definition. A meaningful error will make debugging much easier compared to "Undefined method 'append' for nil:NilClass".

end
end

def select_fields!(by = {})

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

I'm not sure if there's some real-world use-case for this method. I can't imagine that somebody would want to keep just some fields and remove the rest.

It also doesn't work as expected. When I tested it on host info

HammerCLIForeman::Host::InfoCommand.output_definition.select_fields!(:labels => ['Id'], :deep_select => true)

the info command then did contain only two "Id" fields, but none of them displayed any data.

I think it's fine to simple remove this method.

end
end

def replace_field(field_id, fields = nil, &block)

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

This method is handy! I like using the block.
Does it also accept a single field? Using is as replace_field(:name, Field.new(:key => :surname)) would be nice too.

def empty?
@fields.empty?
end

private

def build_definition(fields = nil, &block)

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

We're using similar code at 2 places already. I think we could refactor it into a definition's public method:

def build(dsl = Dsl.new, &block)
  dsl.build(&block) if block_given?
  append(dsl.fields)
end

and also if we changed append to:

def append(fields)
  unless fields.nil?
    fields = [fields] unless fields.is_a? Array
    @fields += fields
  end
end

than it would be much easier for use at all the places and also decoupled from the dsl definition.

This method could then be just:

definition = self.class.new
definition.append(fields)
definition.build(&block)
definition
multiple_replace(i, definition.fields)
end

def at(path = [])

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

I like that method. Could the path also accept a single symbol? Eg at(:interfaces)

deep_select: true/false,
deep_delete: true/false,
keys: [comma separated :field_key or/and :field_id],
labels: [comma separated 'field_label']

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

I guess you could write just "array"

@@ -12,10 +12,117 @@ def append(fields)
@fields += fields
end

def delete_fields!(by = {})
return clear if by.empty?
return deep_delete!(@fields, by) if by[:deep_delete]

This comment has been minimized.

@tstrachota

tstrachota Jun 19, 2018

Member

Use just by[:deep] please. "delete" feels a bit redundant there.

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch from a3f46d1 to 76b9076 Jun 20, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Jun 20, 2018

@tstrachota, updated.

If there is no need for select_fields! method, is there need for delete_fields! method then? I think they are very similar, so I removed it as well. I guess if there will be need to delete a field, it can be done via d.at(path).insert(:replace, field_id_to_delete) do; end

Also now it's possible to insert before/after or replace a field via one single method (see updated docs). I guess it's a good way to avoid code repetition, which in this case will be x3.

@tstrachota
Copy link
Member

tstrachota left a comment

Thanks for you updates @ofedoren. Functionally-wise this is ok. I believe the code can be simplified at several places.

Would you mind describe also append in you docs? I know it's already been there but it's key functionality. It took me some time to find out how to append fields after reading the docs (I nearly reported it as a missing feature ;-)).

end

describe 'find_field' do
# -

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

Missing tests here?

# Expand a field with new definition
HammerCLIForeman::Host::InfoCommand.output_definition
.at(['some', :path])
.insert(:replace, :not_container_field_id, [Fields::Field.new(:first => :one)]) do

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

Nitpick: Examples will be more readable if you split the two ways of using insert:

  • with fields as an argument
  • with dsl in a block

It's not very likely that both ways would be used in combination.

end
# Simplified version
HammerCLIForeman::Host::InfoCommand.output_definition
.insert(:before, :field_id, Fields::Field.new(:first => :one))

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

The docs deserve correct parameters for Fields::Field.new

return self if path.empty?
field = expand_path(@fields, path.dup)
if field.nil? || !field.respond_to?(:output_definition)
raise ArgumentError, 'No output definition at path'

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

It would be very helpful if the error could mention what path was used

index = @fields.find_index do |f|
f.id == field_id
end
raise NameError, 'No such field' if index.nil?

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

Ditto, mentioning the name will make debugging a lot easier.

def at(path = [])
path = [path] unless path.is_a? Array
return self if path.empty?
field = expand_path(@fields, path.dup)

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

If path.dup is there to avoid destructive effect of expand_path, move it inside that method, please.

raise ArgumentError, 'No output definition at path'
end
field.output_definition
end

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

Btw when I'm looking at the method... I think that you can easily drop expand_path and re-use find_field and at recursively. This code should give you the same result:

    def at(path = [])
      path = [path] unless path.is_a? Array
      return self if path.empty?

      field = find_field(path[0])

      if field.nil?
        raise ArgumentError, "Field #{path[0]} not found"
      end
      if !field.respond_to?(:output_definition)
        raise ArgumentError, "Field #{path[0]} doesn't have nested output definition"
      end

      field.output_definition.at(path[1..-1])
    end
dsl.build(&block) if block_given?
append(dsl.fields)
end

def append(fields)

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

Could you please add support for passing &block with dsl here too? I believe that this method is going to be used most so the more convenient it will be the better. It will also bring more consistency with insert.

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

Note: it would also make code in insert simpler:

definition.append(fields, &block)
@fields[field_index(field_id)]
end

def insert(mode = :after, field_id = nil, fields = nil, &block)

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

How about changing the signature to def insert(mode, field_id, fields = nil, &block) ?
Without field_id the method will fail anyway so both parameters are required.

index += 1 if mode == :after
definition = self.class.new
definition.append(fields)
definition.build(&block)

This comment has been minimized.

@tstrachota

tstrachota Sep 20, 2018

Member

This seems too complicated to me. I guess the only reason why you created new definition is the fact that build automatically appends the fields. How about moving injection of Dsl.new into constructor and using just @dsl.build here?

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch from 76b9076 to dff778b Sep 21, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Sep 21, 2018

@tstrachota, updated. Thanks for suggestions for better method definitions!

Updated docs

Also, before it gets merged, I want to be sure if this is an acceptable way of identifying a field by default:

@@ -4,14 +4,16 @@ module Fields

class Field

attr_reader :label
attr_reader :path
attr_accessor :label

This comment has been minimized.

@mbacovsky

mbacovsky Oct 17, 2018

Member

Do we want to enable the writers and allow change these form outside?

This comment has been minimized.

@ofedoren

ofedoren Oct 17, 2018

Author Member

Since we're using labels to set field's ID, which is used to address a field, I don't think it's a good idea now :)

But again, is this an acceptable way? We could allow label changing from outside, but then we shouldn't identify fields by labels I guess..

This comment has been minimized.

@mbacovsky

mbacovsky Oct 17, 2018

Member

Regarding setting the default for id I think it is okay. Maybe with 'key' preferred over the 'label'.
In the original comment I was referring more to all three attr_accessors in general. It does not feel correct to modify any of these attributes on existing field. Wouldn't it be better to replace filed with a new field?

I can imagine changing label in container or collection could be awkward, so ability to change label might be helpful in some cases. The :id would need to be a method getting the id dynamically.

@@ -20,6 +20,7 @@ def build(&block)
def field(key, label, type=nil, options={}, &block)
options[:path] = current_path.clone
options[:path] << key if !key.nil?
options[:field_id] = options[:field_id] || key || options[:key]

This comment has been minimized.

@mbacovsky

mbacovsky Oct 17, 2018

Member

This is inconsistent with the Field class defaults. If field is created directly without the :field_id the default id is 'label'. Via dsl the 'key' wins. What about not setting the default value in here and leave it up to Field class?


def initialize(options={})
@hide_blank = options[:hide_blank].nil? ? false : options[:hide_blank]
@hide_missing = options[:hide_missing].nil? ? true : options[:hide_missing]
@path = options[:path] || []
@label = options[:label]
@id = options[:field_id] || label || options[:key]

This comment has been minimized.

@mbacovsky

mbacovsky Oct 17, 2018

Member

Is there a reason the option is called :field_id and the attribute just id? It is not consistent with other options.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 17, 2018

@ofedoren, I've left some comments and questions inline. Besides that it looks nice and powerful.

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch from 19b60b8 to c4f3594 Oct 18, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Oct 18, 2018

Updated with @mbacovsky suggestions.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 19, 2018

@ofedoren thanks for quick update!

I'm sorry I didn't noticed earlier but I played with the feature and wanted to found out how it behaves when the id is not found. Similarly as in help extension it raises error which prevents whole plugin from loading.

I'd prefer it to print warning, log whatever we have for debuging later and print as much as we can. The reason is to prevent e.g. one removed field in hammer-cli-foreman from loading whole other plugin.
It may be a bit difficult to handle the errors with the current notation. Would it help to make it similar to extend_help and wrap it into a block like: CommandXY.extend_output_definition do |od|; end?

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch from c4f3594 to 72ad363 Oct 24, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Oct 24, 2018

@mbacovsky, updated.

I added handling similar to one from help extension. So, if a field is not found, then the whole output extension block will fail, but the plugin will be loaded and other extension blocks will be applied (if there are no errors).

Also, updated docs.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 30, 2018

[test hammer]

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 31, 2018

@ofedoren a way better, thanks!

While testing I found another issue. I had the :mark_translations settings turned on so my labels were modified to e.g. >Name< and I realized addressing by labels won't work for localized Hammer. But there is not much we can do about it, right?

I also found out that when the field is created via dsl it won't have the :key set which limits the possibilities to identify the fields. The following patch fixed it for me:

diff --git a/lib/hammer_cli/output/dsl.rb b/lib/hammer_cli/output/dsl.rb
index e0cf7ab..5cf6a38 100644
--- a/lib/hammer_cli/output/dsl.rb
+++ b/lib/hammer_cli/output/dsl.rb
@@ -19,7 +19,10 @@ module HammerCLI::Output
 
     def field(key, label, type=nil, options={}, &block)
       options[:path] = current_path.clone
-      options[:path] << key if !key.nil?
+      unless key.nil?
+        options[:path] << key
+        options[:key] = key
+      end
 
       options[:label] = label
       type ||= Fields::Field

Otherwise it looks good!


def field_index(field_id)
index = @fields.find_index do |f|
f.id == field_id

This comment has been minimized.

@mbacovsky

mbacovsky Oct 31, 2018

Member

I had an idea to match the field by ANY of those identifiers like:

f.match_id?(field_id)

where the matcher is defined as

def match_id?(field_id)
  @options[:id] == filed_id || @options[:key] == field_id || @label == field_id
end

It could prevent breaking the extension when e.g. the field_id is added.
What do you think?

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch from 72ad363 to 0bae9e8 Nov 7, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Nov 7, 2018

@mbacovsky, updated. Thanks for match_id? suggestion! It makes more sense to use it instead of simple ==.

Also, I'm not sure if there is a problem with addressing by labels for localized Hammer. It seems to be working if address with _(), e.g. use .find_field(_('Name')) instead of .find_field('Name').

end

def match_id?(field_id)
@options[:id] == field_id || @options[:key] == field_id || @label == field_id

This comment has been minimized.

@tstrachota

tstrachota Nov 7, 2018

Member

Would it make sense to add || @label == _(field_id) here? It could seamlessly solve the issue with translations.

@ofedoren ofedoren force-pushed the ofedoren:feat-21634-modif-output-fields branch from 0bae9e8 to f8173ad Nov 7, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Nov 7, 2018

@tstrachota, updated.

@tstrachota
Copy link
Member

tstrachota left a comment

I'm good. @mbacovsky ?

@@ -146,4 +146,3 @@ def self.main_makefile

domain = [HammerCLI::I18n::LocaleDomain.new, HammerCLI::I18n::SystemLocaleDomain.new].find { |d| d.available? }
HammerCLI::I18n.add_domain(domain) if domain

This comment has been minimized.

@tstrachota

tstrachota Nov 9, 2018

Member

Please avoid changing files that aren't affected by the purpose of the patch.

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 9, 2018

[test hammer]

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 28, 2018

@mbacovsky ping

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 30, 2018

[test hammer]

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 30, 2018

All comments have been addressed and the changes work fine. Merging, thanks @ofedoren this is an important feature!

@tstrachota tstrachota merged commit 32a2651 into theforeman:master Nov 30, 2018

1 check passed

hammer Build finished. 2217 tests run, 0 skipped, 0 failed.
Details
@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.