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 #21633 - Allow modification of additional help texts #277

Merged
merged 1 commit into from Nov 1, 2018

Conversation

Projects
None yet
4 participants
@ofedoren
Copy link
Member

ofedoren commented Jun 18, 2018

Currently the help string builds on the fly to actual string, so it is not so easy to change it externally. I suggest to build a sort of skeleton or some structure with definitions as it is with command's output_definition at first and build the help string at the end of all of the extensions/modifications.

Even though help now builds via help.definition.build_string, I left everything in help/text_builder for backwards compatibility, which leads to some code duplications in help/definition.

Here the docs.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Sep 2, 2018

For the first rough check the code looks well built and I appreciate the help and tests. Before I get to detailed review I'd like to rise my concerns with the design.
@ofedoren, @tstrachota I'm a bit concerned about the flexibility we provide and that with plugins it could easily get out of hands. Namely I'm concerned about possibility to remove and change content and addressing the content by index. This could easily lead to removal of wrong content by plugins.

I'd suggest to allow only addition to the end of the definition items that has an id (section and list). That should provide enough control to change update the help. As an user I'd prefer information "With plugin xy it is not possible to do abc" over just removing "abc" from the help. This provides more information about why certain feature is not available and what caused that.

Would that be reasonable change to request?

@tstrachota
Copy link
Member

tstrachota left a comment

I like how you designed help components 👍

I do share @mbacovsky's concerns about using indexes for addressing the components. It's extremely fragile. IDs should be enough, especially when we set default IDs from labels.

I'm ok with keeping delete and replace. They'll rarely be used (if at all) but they don't hurt.

Please see my inline comments. I identified some dead code and I also think we need more error reporting. At the moment when you use wrong id or wrong keys for the option hashes, it just fails silently and that's quite frustrating.

end
end
! Prefer relative positions to ids because not all of sections/lists/text have

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

Actually I think it should be preferred to use ids where possible and appending elsewhere. Relative positions can be quite fragile.


protected

def indent(content, indentation = nil)

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

Exactly the same function is in TextBuilder class. I think it makes sense to extract it and make the code more DRY.

end

def string
@out.string
end

def text(content)
def text(content, options = {})
puts unless first_print?
puts content

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

I don't understand why all the puts are still preserved. It seems to me that everything is printed regardless of the changes you make on the help structure.

@@ -53,6 +64,36 @@ def indent(content, indentation = nil)
end.join("\n")
end

def at(mode = :id, path = [])

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

This method definitely needs to print or log warnings when the desired node wasn't found. Otherwise it will be hell to debug.

We should also consider using the same signature as for replace and delete to make usage consistent:

at(id: 'Libvirt')

(If we want to keep paths at all)

end.first
end
return elem if path.empty? || !elem.respond_to?(:definition)
expand_path(mode, elem.definition, path)

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

Maybe it's just me but this method is difficult to read.

@@ -0,0 +1,109 @@
module HammerCLI
module Help
class Definition < Array

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

👍 This is nice inherritance. The file itself is becoming quite long. It's worth considering to split it per class.

private

def expand_path(mode, definition, path)
id = path.shift

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

I'd recommend ensuring the path is an array here. Not a bad idea to support also single ids.

help_extension_blocks.each do |extension_block|
extension_block.call(help_extension)
end
builder.add_text(help_extension.definition.build_string)

This comment has been minimized.

@tstrachota

tstrachota Sep 19, 2018

Member

Unless there's some special reason for changing it, I'd keep the help_extension's interface as it was and use #string method.

@ofedoren ofedoren force-pushed the ofedoren:feat-21633-modif-help-text branch from 79420ed to 60beac5 Sep 28, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Sep 28, 2018

@tstrachota, @mbacovsky updated.

I refactored a bit previous version as you suggested with some additional changes. Also I removed the possibility to address the content by index. delete and replace methods are replaced with insert. In general modification of additional help text now works similar to modification of output fields.

Updated docs.

@ofedoren ofedoren force-pushed the ofedoren:feat-21633-modif-help-text branch from 60beac5 to 2bf882a Oct 10, 2018

@mbacovsky
Copy link
Member

mbacovsky left a comment

@ofedoren, well done! I like the consistency with output modifications. I still don't think removing and replacing help from an extensions is a good practice ;) but the tool is pretty flexible. Extra kudos for the help 🍎!

The only thing I'm going to ask to change is handling of lookup failures. It shouldn't prevent us to display as much help as we can. Warning and logging of details to identify the extending plugin should suffice.

index = find_index do |item|
item.id == item_id
end
raise ArgumentError, "Help item #{item_id} not found" if index.nil?

This comment has been minimized.

@mbacovsky

mbacovsky Oct 18, 2018

Member

It may be better to wrap the item_id in apostrophes. When item_id is the section label it would be easier to find in the message.

It may be better user experience to catch the error at some point, print a warning that the attempt to extend the help failed on missing item_id and log the backtrace but go on and print as much help as we can. That way outdated plugin trying to extend the help couldn't break the execution.

@ofedoren ofedoren force-pushed the ofedoren:feat-21633-modif-help-text branch from adbfc92 to 933238b Oct 24, 2018

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Oct 24, 2018

@mbacovsky, updated.

If any Help item not found error is raised, it will be handled with default handler. So, the error is logged and nothing breaks, it just skips the block caused error.

Please tell me if the error message user sees is good enough or it should be changed, so I add a similar handling to #275

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 24, 2018

[test hammer-cli]

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 24, 2018

[test hammer]

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 24, 2018

@ofedoren that looks pretty well now. Is it a final version that can be merged?

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 24, 2018

The failing tests are not related, I'll rerun later.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Oct 24, 2018

@tstrachota could you revisit, please?

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Oct 24, 2018

@mbacovsky yes, if there are no additional proposes, it is the final version.

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 1, 2018

[test]

@tstrachota

This comment has been minimized.

Copy link
Member

tstrachota commented Nov 1, 2018

Looks good to me. Test failure is most likely unrelated but I re-triggered them just to be sure.

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Nov 1, 2018

[test hammer]

@mbacovsky

This comment has been minimized.

Copy link
Member

mbacovsky commented Nov 1, 2018

@mbacovsky mbacovsky merged commit 99cf9e5 into theforeman:master Nov 1, 2018

1 check passed

hammer Build finished. 2136 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
You can’t perform that action at this time.