Bug #6752

Allow action-specific render methods

Added by Paul Berry about 1 year ago. Updated 11 months ago.

Status:Closed Start date:03/17/2011
Priority:Normal Due date:
Assignee:Daniel Pittman % Done:

0%
Category:Faces
Target version:2.7.0
Affected Puppet version:development Branch:https://github.com/daniel-pittman/puppet/commits/bug%2F2.7.x%2F6752-allow-action-specific-render-methods
Keywords:
Votes: 1

Description

The current implementation of interfaces allows a render method to be specified at the interface level but not at the action level. (The render method is in fact specified in the application file, not the interface file).

This is unfortunate because not all actions in the same interface generate the same kind of output, so it does not necessarily make sense to have their output rendered in the same way. For example, “puppet catalog find” generates a catalog, which should probably be rendered as YAML or JSON, whereas “puppet catalog select” generates a list of resources.

It would be nice if interface-specific render methods could be specified in the declaration of the interface, and action-specific render methods could be specified in the declaration of the action.

The current implementation works around this situation by having “puppet catalog select” direct its output straight to stdout (instead of returning it)—this means that “puppet catalog select” cannot be effectively used from within Ruby.

History

#1
Updated by Dan Bode about 1 year ago

This also makes adding plugin actions to an existing interface impossible as well.

Ex:

I would like to add an action ‘diff’ to an existing interface ‘catalog’, however, and I cannot use the existing rendering, so I had to create a new interface

#2
Updated by Paul Berry about 1 year ago

I believe this ticket will be rendered unnecessary by ticket #6781. Some of Dan’s logic may need to be moved from the render methods to the individual actions, but I think this would be a beneficial move.

#3
Updated by Paul Berry about 1 year ago

  • Category changed from 21 to Faces

#4
Updated by Dan Bode about 1 year ago

“I believe this ticket will be rendered unnecessary by ticket #6781. Some of Dan’s logic may need to be moved from the render methods to the individual actions, but I think this would be a beneficial move.”

I strongly disagree, examples:

  1. I would never want to set exit codes in the interface, it has nothing to do with the data being abstracted

  2. I already have several examples where I am transforming the object representation of something into something only useful for printing. I am adding headers, and even transforming the data for better readability. I can assert that this proposed change would make it harder for me to code interfaces/applications. It would basically require that I created a class for every interface and implement to_s, this is a lot of overhead to introduce compared to the render method.

#5
Updated by Dan Bode about 1 year ago

my concerns about this could be resolved by #6814

being able to specify actions as an object could provide the ability to do custom rendering and setting exit codes as methods on the instance.

#6
Updated by Daniel Pittman about 1 year ago

At a design level:

What is the concrete requirement here? Would it be sufficient to inspect the action object in the render method and do your own dispatch, or do you want to have per-action render methods?

Do you want to have a default render, and then override it for a subset of actions?

How do we propose to handle actions supplied by the end user, and rendering those? They are an unknowable set while we write this code.

#7
Updated by Daniel Pittman about 1 year ago

  • Tracker changed from Feature to Bug

OK, after discussing this with a bunch of people we identified the following specific requirements:

  • rendering for humans must be available to third parties extending faces.
  • rendering for humans must be available when adding an action to an existing face.
  • rendering for humans should not require adding a second file (eg: inline rendering.)
  • rendering for humans must not break users of the Ruby API.
  • rendering for humans must not break faces over non-CLI (eg: web, STOMP, MCollective.)

To implement this we have the proposed API:

action :example do
  when_invoked do |a, b, c|
    # whatever
    return my_data
  end

  when_rendering :for_humans do |returned_data|
    # perform whatever transformation you desire on returned_data here, and...
    return "human friendly version of the data"
  end
end

This is an extensible model: you can also hook, for example, when_rendering :json using this model, and perform similar arbitrary transformations based on the target format, without requiring users to request it.

Additionally, we will have a default_format for the face, and perhaps the action, that will default to :for_humans when the CLI is run, but will default to :json or some other machine-focused format when the HTTP or STOMP API is in force. (A global CLI switch to prefer machine format data will be added, too.)

Faces that specify an explicit default format will just use that, whatever facade they have.

Finally, we will implement some sensible default rendering in the command line for folks who don’t specify their own rendering hook:

  • return a String and you get printed directly, no modification.
  • return a Hash and you get printed as a “table” to the screen. (no bars, just whitespace alignment.)
  • return anything else (including an array) and you get pp object printed

That has fairly reasonable behaviour out of the box, presents useful data as well as we can, and makes it easy for people to do useful transformation in their render methods, too. Also, it means that if someone is enthused to return a rich object and implement pretty_print they get the right™ behaviour there, too.

The only thing undecided at this point is what to call the :for_humans rendering mode; I have no better idea, but don’t over-much like that name. Suggestions welcome.

#8
Updated by Dan Bode about 1 year ago

I am happy with this proposal

how about pretty_print?

#9
Updated by Daniel Pittman about 1 year ago

Also, it means that if someone is enthused to return a rich object and implement pretty_print they get the right™ behaviour there, too.

…or did you mean something other than the pretty_print hook provided by the pp module?

#10
Updated by Daniel Pittman about 1 year ago

  • Assignee set to Daniel Pittman
  • Target version changed from 2.6.x to 2.7.x
  • Affected Puppet version set to development
  • Branch set to https://github.com/daniel-pittman/puppet/commits/bug%2F2.7.x%2F6752-allow-action-specific-render-methods

https://github.com/daniel-pittman/puppet/commits/bug%2F2.7.x%2F6752-allow-action-specific-render-methods contains my work in progress branch for this; it implements the smart human rendering mechanism, wired in to replace the older default behaviour where no default format was specified.

This probably needs some work on the smart rendering, and then needs to be wired through the rest of the system.

The only outstanding question about the current rendering is that keys can be unlimited length; I propose capping them at 38 characters, and using ellipses in shortening long strings. That should give a reasonable balance of utility and data trimming for human-focused output.

#11
Updated by Luke Kanies about 1 year ago

Daniel Pittman wrote:

[…] The only outstanding question about the current rendering is that keys can be unlimited length; I propose capping them at 38 characters, and using ellipses in shortening long strings. That should give a reasonable balance of utility and data trimming for human-focused output.

Why? That is, is it really worth investing in infrastructure to make it so people can’t use options longer than 38 characters? Just refuse any patches that actually do this in core, and let people do whatever they want in their own system. Is that not sufficient?

#12
Updated by Daniel Pittman about 1 year ago

Luke Kanies wrote:

Daniel Pittman wrote:

[…] The only outstanding question about the current rendering is that keys can be unlimited length; I propose capping them at 38 characters, and using ellipses in shortening long strings. That should give a reasonable balance of utility and data trimming for human-focused output.

Why? That is, is it really worth investing in infrastructure to make it so people can’t use options longer than 38 characters? Just refuse any patches that actually do this in core, and let people do whatever they want in their own system. Is that not sufficient?

Er, perhaps you misunderstand me: this is about displaying the text, not using it. It seemed worth asking the question about the line of code required to truncate the keys to me, so we had thought about user-focused presentation. None of this would be about preventing people using the keys anywhere in the system or anything.

So, no, other than the question of intent, it didn’t seem like either a lot of work – gsub(/(.{1,35}).*$/) { |x| x[0] + ‘…’ } – to support this.

#13
Updated by Daniel Pittman about 1 year ago

I have pushed an updated preview of this branch to my repository, spreading support for this further through the code. Now to wire up the CLI interface.

#14
Updated by Daniel Pittman about 1 year ago

I have what I believe to be a final version of this code pushed, awaiting review.

#15
Updated by Daniel Pittman about 1 year ago

  • Status changed from Accepted to Merged - Pending Release

https://github.com/puppetlabs/puppet/commit/7438723f6ae13605a48c5db63839a829a19f5127 merges this into the 2.7.x branch.

This is now ready for use, and should apply cleanly enough to existing code.

#16
Updated by Josh Cooper about 1 year ago

  • Status changed from Merged - Pending Release to Closed

#17
Updated by James Turnbull 11 months ago

  • Target version changed from 2.7.x to 2.7.0

Also available in: Atom PDF