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 #21590 - Added error to wrong --output #315

Merged
merged 2 commits into from Sep 10, 2019

Conversation

@yifatmakias
Copy link
Contributor

yifatmakias commented Sep 9, 2019

No description provided.

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Sep 9, 2019

Can one of the admins verify this patch?

@yifatmakias yifatmakias force-pushed the yifatmakias:21590 branch from aa8a204 to eefea08 Sep 9, 2019
@yifatmakias yifatmakias changed the title Fixes 21590 - Added print_output_type function Fixes #21590 - Added print_output_type function Sep 9, 2019
Copy link
Member

ofedoren left a comment

Thanks for contributing, @yifatmakias!

Although this addition doesn't prevent the users to run a command by showing an error, I think the better approach will be preventing with showing the right usage, since we don't support other output adapters.

I've already (accidentally) solve it as a part of #312 (see https://github.com/theforeman/hammer-cli/pull/312/files#diff-ebbb17d97d1fa6482776d9c9cad6c3fdL50-L54). If you agree that that's the better approach from the user perspective, you are free to use it within your PR :)

Also, some nitpicks inline and I think if we're going to stick with your solution, we could get rid of theforeman/hammer-cli-foreman#446 by adding output.print_output_type here.

lib/hammer_cli/output/output.rb Outdated Show resolved Hide resolved
@yifatmakias yifatmakias changed the title Fixes #21590 - Added print_output_type function Fixes #21590 - Added error to wrong --output Sep 10, 2019
Copy link
Member

ofedoren left a comment

Good update, @yifatmakias!

Just a few small nitpicks (see inline).

lib/hammer_cli/main.rb Outdated Show resolved Hide resolved
lib/hammer_cli/main.rb Outdated Show resolved Hide resolved
@ofedoren

This comment has been minimized.

Copy link
Member

ofedoren commented Sep 10, 2019

[test]

@ofedoren

This comment has been minimized.

Copy link
Member

ofedoren commented Sep 10, 2019

[test hammer]

Copy link
Member

ofedoren left a comment

All good now, thanks, @yifatmakias! Merging.

@ofedoren ofedoren merged commit 3fcf76d into theforeman:master Sep 10, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 2400 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.