Skip to content

Conversation

@thaJeztah
Copy link

  • do an early check if a custom format is specified either through the
    command-line, or through the cli's configuration, before adjusting
    the options (to add "size" if needed).
  • if no format is set after this, set the default (formatter.TableFormatKey)
  • also removes a redundant options.Size = opts.size line, as this value is
    already copied at the start of buildContainerListOptions()

@thaJeztah
Copy link
Author

@photra ptal; if you agree with these changes, and merge this PR, it will be included in your PR ☺️👍

@photra
Copy link
Owner

photra commented Jun 7, 2022

Hi @thaJeztah, I just made some suggestions to above for conciseness. Please let me know what you think about it! 😁

@thaJeztah thaJeztah force-pushed the 3632_fix_ps_format_suggestions branch from 9d1b7a0 to c51ef56 Compare June 7, 2022 08:19
@thaJeztah
Copy link
Author

@photra I pushed a change; added one extra commit in which I fixed the logic in NewContainerFormat (keeping it as a separate commit for now to make it easier to find back)

- do an early check if a custom format is specified either through the
  command-line, or through the cli's configuration, before adjusting
  the options (to add "size" if needed).
- if no format is set after this, set the default (formatter.TableFormatKey)
- also removes a redundant `options.Size = opts.size` line, as this value is
  already copied at the start of buildContainerListOptions()
- Update NewContainerFormat to use "table" format as a default if no format
  was given.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the 3632_fix_ps_format_suggestions branch from c51ef56 to 94fd3d9 Compare June 7, 2022 08:30
@thaJeztah
Copy link
Author

Actually, perhaps we should do the last commit as a follow-up for visibility; let me remove it for now (and I can push it as a separate PR); possibly it needs discussion as there will be a slight change in behavior, which may need discussion.

I removed the commit, and included some of the change in the existing one 👍

@photra
Copy link
Owner

photra commented Jun 7, 2022

Gotcha, merging this PR now!

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