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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions doc/commands_modification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
Modify an existing command
-------------------------

### Modification of output fields

Each command (as well as each field) might have its own
output definition. You can modify the output definition of existing
command or a specific field of that command by following output
definition interface:

```ruby
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
# Appends fields to the end.
# Where:
# fields is one or more existing fields
# block is block of code with new fields
definition.append(fields) do
label
field
collection
end
# Inserts one or more fields.
# Where:
# :mode is one of [:before, :after, :replace]
# :id is field's id or key. Field's label can be used if field does not
# have id
# fields is one or more existing fields
# block is block of code with new fields
definition.insert(:mode, :id, fields) do
label
field
collection
end
# Returns output definition of the command or field specified with path.
# Where:
# path = Array of :key or/and :id or/and 'label']
definition.at(path)
# Returns field from current output definition.
definition.find_field(:id)
# Deletes all fields from current output definition.
definition.clear
end
```
#### Examples
```ruby
# Append some fields to the end
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
definition.at(_('Collection Field Label'))
.append do
field :value3, _('Value 3')
end
end
# Change field's label
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
definition.at(:path)
.find_field(:id).label = _('New label')
end
# Expand a field with new definition
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
definition.at([_('some'), :path])
.insert(:replace, :not_container_field_id) do
field nil, _('Label with new fields'), Fields::Label, id: :now_container_field_id do
field :new_field1, _('New field 1')
field :new_field2, _('New field 2')
end
end
end
# Insert a new field after specific field
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
definition.at([_('some'), :path])
.insert(:after, :other_field_id, [Fields::Field.new(label: _('My field'), id: :my_field_id)])
end
# Insert a new field before specific field
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
definition.insert(:before, :other_field_id,
Fields::Field.new(label: _('My field'), id: :my_field_id))
end
# Remove field
HammerCLIForeman::Host::InfoCommand.extend_output_definition do |definition|
definition.insert(:replace, :field_id) do; end
end
```
1 change: 1 addition & 0 deletions doc/developer_docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Contents:
- [Writing a plugin](writing_a_plugin.md#writing-your-own-hammer-plugin)
- [Creating commands](creating_commands.md#create-your-first-command)
- [Help modification](help_modification.md#modify-an-existing-help)
- [Commands modification](commands_modification.md#modify-an-existing-command)
- [Option builders](option_builders.md#option-builders)
- [Creating ApiPie commands](creating_apipie_commands.md#creating-commands-for-restful-api-with-apipie)
- [Development tips](development_tips.md#development-tips)
Expand Down
7 changes: 7 additions & 0 deletions lib/hammer_cli/abstract.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,13 @@ def self.extend_help(&block)
self.help_extension_blocks << block
end

def self.extend_output_definition(&block)
block.call(output_definition)
rescue ArgumentError => e
handler = HammerCLI::ExceptionHandler.new
handler.handle_exception(e)
end

def self.output(definition=nil, &block)
dsl = HammerCLI::Output::Dsl.new
dsl.build &block if block_given?
Expand Down
1 change: 0 additions & 1 deletion lib/hammer_cli/i18n.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Member

Choose a reason for hiding this comment

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

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

55 changes: 51 additions & 4 deletions lib/hammer_cli/output/definition.rb
Original file line number Diff line number Diff line change
@@ -1,21 +1,68 @@
module HammerCLI::Output

class Definition

attr_accessor :fields

def initialize
@fields = []
end

def append(fields)
def append(fields = nil, &block)
fields = [fields].compact unless fields.is_a?(Array)
@fields += fields
return @fields unless block_given?
dsl = Dsl.new
dsl.build(&block)
@fields += dsl.fields
end

def find_field(field_id)
@fields[field_index(field_id)]
end

def insert(mode, field_id, fields = nil, &block)
index = field_index(field_id)
index += 1 if mode == :after
definition = self.class.new
definition.append(fields, &block)
insert_fields(index, definition.fields, with_remove: mode == :replace)
end

def at(path = [])
Copy link
Member

Choose a reason for hiding this comment

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

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

path = [path] unless path.is_a? Array
return self if path.empty?

field = find_field(path[0])

unless 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
Copy link
Member

Choose a reason for hiding this comment

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

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


def clear
@fields = []
end

def empty?
@fields.empty?
end

end
private

def field_index(field_id)
index = @fields.find_index do |f|
f.match_id?(field_id)
end
raise ArgumentError, "Field #{field_id} not found" if index.nil?
index
end

def insert_fields(index, fields, with_remove: false)
@fields.delete_at(index) if with_remove
fields.each_with_index do |f, i|
@fields.insert(index + i, f)
end
end
end
end
6 changes: 4 additions & 2 deletions lib/hammer_cli/output/dsl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ def build(&block)

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
custom_field type, options, &block
Expand Down
11 changes: 9 additions & 2 deletions lib/hammer_cli/output/fields.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@
module Fields

class Field

attr_reader :label
attr_reader :path
attr_accessor :label
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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..

Copy link
Member

Choose a reason for hiding this comment

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

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.


def initialize(options={})
@hide_blank = options[:hide_blank].nil? ? false : options[:hide_blank]
Expand All @@ -15,6 +14,14 @@ def initialize(options={})
@options = options
end

def id
@options[:id] || @options[:key] || @label
end

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

def hide_blank?
@hide_blank
end
Expand Down
Loading