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

Use plaintext instead of table for human-readable output format. #1803

Merged
merged 6 commits into from
Oct 22, 2021

Conversation

joshuabezaleel
Copy link
Contributor

What does this change

Use plaintext instead of table for human-readable output format.

What issue does it fix

Closes #1491

Checklist

  • Unit Tests
  • Documentation
  • Schema (porter.yaml)

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
@carolynvs carolynvs self-assigned this Oct 18, 2021
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Looks great! There's just one thing that needs fixing and we should be all set.

Also it looks like there is a merge conflict with the release/v1 branch. Let me know if you need help resolving that.

* [Om More](https://github.com/thisisommore)
* [Joshua Bezaleel Abednego](https://github.com/joshuabezaleel)
Copy link
Member

Choose a reason for hiding this comment

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

Yay! 🎉

@@ -98,21 +96,7 @@ func (p *Porter) PrintInstallationRuns(opts RunListOptions) error {
return printer.PrintYaml(p.Out, displayRuns)
case printer.FormatPlaintext:
return printer.PrintPlaintext(p.Out, displayRuns)
case printer.FormatTable:
Copy link
Member

Choose a reason for hiding this comment

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

Oh! I just realized that we have a function defined printer.PrintPlaintext that is just totally wrong and we shouldn't be using it.

Can you remove the printer.PrintPlaintext function and then add back this case in the switch statement so that it prints the run properly when Plaintext is the selected format? The current logic that we have here for table is what we want to keep.

--------------------------------------------------------
1 install 2020-04-18 0001-01-01 succeeded
2 uninstall 2020-04-18 0001-01-01 succeeded
[{1 install map[] 2020-04-18 01:02:03.000000004 +0000 UTC 0001-01-01 00:00:00 +0000 UTC succeeded} {2 uninstall map[] 2020-04-18 01:02:03.000000004 +0000 UTC 0001-01-01 00:00:00 +0000 UTC succeeded}]
Copy link
Member

Choose a reason for hiding this comment

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

The output here is why I'm asking for the change above (adding back the original table case statement to that switch statement). As you can see printer.PrintPlaintext just dumps the data.

So the changes to this file should be reverted and then make sure that this test case passes.

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
@joshuabezaleel
Copy link
Contributor Author

@carolynvs PR updated, thank you for the always kind and detailed review Carolyn! 😄

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

YES! 💯 This is one of my pet peeves with Porter, the mixed up format names. So thank you for fixing it.

@carolynvs carolynvs merged commit e24b32e into getporter:release/v1 Oct 22, 2021
@joshuabezaleel joshuabezaleel deleted the rename-table-plaintext branch October 23, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants