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 #25878 - New lines in text attr dont break output #300

Merged

Conversation

@ofedoren
Copy link
Member

ofedoren commented Apr 2, 2019

The fix adds a possibility to print long text (e.g. descriptions) with line breakers into one line. Simply changes new lines into spaces. This can be achieved with inline: true option for LongText field.

JSON and YAML output is not changed, since all whitespaces are escaped there.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 3, 2019

@ofedoren thanks for the PR! I tested it and it works as expected. However I'm not bought for the way it is implemented. Maybe we can use this issue to make the Adapters - Formatters - Tags more understandable and better documented. It is a pain for a long time and it takes too much affort to figure out how it works every time we need to change something. I did the exercise again and here is what I re-learned and hopefully base for some documentation:

Adapter is responsible for rendering the output in a specific way (table, csv, yaml).
Field is an object that holds the piece of information the output consists form. It has a Type, Label and Value.
Formatter is a bit of code that can modify a representation of a field during output rendering. Formatter is registered for specific field type. Each field type can have multiple formatters.

As the adapters vary significantly in the output structure and purpose we need to limit some formatters for specific adapters only. Hammer is designed to allow adding new adapters also from plugins so we need something flexible to bind the formatters correctly even to possible future adapters. The Tags serves for this purpose. Maybe adapter category would be better name for it. In practice the adapters have tags and the formatters have the same tags too. Formatter is applied during rendering of the field only if all the tags it has are also in the target adapter.

E.g. RedNegativeFormatter that prints negative numbers in red for NumericFields would have tags :flat and :screen. It will be applied by :table adapter that has the same tags but won't be applied by :csv adapter that has only :flat tag.

Currently used tags are of two kinds. The first one help us to align by structure of the output. :flat means the fields are serialized into a string (:table, :csv, :base adapters), :data means the output is structured (:yaml and :json adapters). The other kind serves to distinguish the cases where we can use the xterm colors to improve the output and the relevant tags are :screen and :file. For this group the :table and :basic adapters have the :screen tag (:file is unused yet, maybe was intended as non-interractive output). This makes it possible that the RedNegativeFormatter from our example is applied only by :table and :base adapters and our xterm color escape sequences do not pollute the output in CSV or YAML.

The tags are symbols and can be freely added e.g. by plugins when necessary. We can have plugin adding XML adapter and some specific XML related formatters and binding them with a :xml tag.


So back to the PR - we should avoid adding :screen tag to :csv adapter. To dispatch the formatter correctly we may add new tag :inline that would indicate that the value will render into single line without newlines. Good candidates for this would be :table and :csv adapters. In :base adapter I think we may still print description over multiple lines though properly indented. For this :multiline tag could serve. then we could have two formatters one for :flat,:inline and one for :flat,:multiline.

Would it make sense? Are there any better solutions?

Also do we want to re-design our tags to make it cleaner? As probably no third-party adapters were added by plugins since the start of the project we can resign on the flexibility and list adapters the formatter is applicable in in each formatter (instead of the tags). That could make it clean and clear. on the other hand adding new adapter would require update in plugins that are adding own formatters for their specific value types.

@ofedoren ofedoren changed the title Fixes #25878 - New lines in text attr dont break output [WIP]Fixes #25878 - New lines in text attr dont break output Apr 5, 2019
@ofedoren ofedoren force-pushed the ofedoren:bug-25878-new-lines-in-text-attr branch 2 times, most recently from d100811 to ac6bbb8 Apr 5, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Apr 8, 2019

@mbacovsky, updated.

I've added two additional formatters as you suggested and some documentation about adapters, formatters, tags: https://github.com/ofedoren/hammer-cli/blob/ac6bbb8bf2d0388aa53d7bd2fd783043f57c2464/doc/output.md

Also, I've refactored tags:

If you don't like to have two names for pretty same thing, I'd suggest another one: style_requirements for both.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 11, 2019

@ofedoren amazing job, thanks! I'll do some testing but no objections so far!

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 12, 2019

@ofedoren I almost merged this but I realized we have also formatters in hammer plugins and I believe renaming tags will break data rendering there. I guess we can keep the tags method and read it from the limitation and `feature methods. What do you think?

Also the limitation and feature was a bit hard to get at a first sight as I see what you call the limitation to be rather a feature of the adapter. And the feature of the formater to be required_feature for the formatter to work. Anyway what you've suggested is far better then the tags and we ca keep it as is.

Your approach made me to look at it from different perspective and maybe it could be pushed even further if we renamed the tags/features/limitations:

flat -> serialized
data -> structured
screen -> rich_text (?)
file -> plain_text (?)
inline
multiline

but the question is if the added value is worth the troubles with keeping the plugins compatible.

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Apr 12, 2019

@mbacovsky, thanks for another detailed review :) I agree that renaming adapter's limitations to features as well as formatter's features to required_features makes more sense and I'd rather rename it in this way. Also, it's not a problem for me to create Ref PRs to all plugins within project that use tags and rename it.

I've also thought about renaming flat/data etc to something more meaningful...

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Apr 12, 2019

@ofedoren sounds great!
In the past I've heard of some plugins outside our project so it would be fair to give them chance to keep up with the breaking changes. If we fix plugins in our project it would be possible to deprecation warning that would be unobtrusive for our users and warn those whose plugins we missed.

@ofedoren ofedoren force-pushed the ofedoren:bug-25878-new-lines-in-text-attr branch from ac6bbb8 to 0f2495a May 21, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented May 21, 2019

@mbacovsky, updated. I've made PRs to plugins where I found tags method usage:

Also, a warning message will be printed if tags method was defined:

_('Method %{tags} for field formatters and output adapters is deprecated. Please use %{feat} or %{req_feat} instead.') % {

@ofedoren ofedoren changed the title [WIP]Fixes #25878 - New lines in text attr dont break output Fixes #25878 - New lines in text attr dont break output Jun 13, 2019
Copy link
Member

mbacovsky left a comment

Well done, thanks for those updates @ofedoren. Merging finally!

@mbacovsky mbacovsky merged commit a4dcbe7 into theforeman:master Sep 23, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 2379 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.