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

cmd/image/tree: refactor #5572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laurazard
Copy link
Member

- What I did

Just some refactors to the image tree formatting logic to make reuse/reading easier. Consolidates the printing in one place, and reduces how far down we need to pass our writer.

This also makes testing these functions less involved.

- How I did it

(wip, I need to push more commits)

Make most of the formatting functions return strings, instead of printing directly.

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes missing coverage. Please review.

Project coverage is 59.60%. Comparing base (da9e984) to head (4f849b4).
Report is 34 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5572      +/-   ##
==========================================
+ Coverage   59.15%   59.60%   +0.44%     
==========================================
  Files         342      345       +3     
  Lines       29079    29102      +23     
==========================================
+ Hits        17203    17346     +143     
+ Misses      10901    10787     -114     
+ Partials      975      969       -6     

Comment on lines +240 to +241
out += printNames(columns, img, topNameColor, untaggedColor)
out += printDetails(columns, normalColor, img.Details)
Copy link
Member

Choose a reason for hiding this comment

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

Slightly on the fence on this, because it means that we'd now be buffering all output in a string, instead sending it to the writer 🤔

Copy link
Member Author

@laurazard laurazard Oct 24, 2024

Choose a reason for hiding this comment

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

I didn't think that was a big deal for this output, but I can change it back. This isn't really in the hotpath, and makes reuse easier. Otherwise, if we want to reuse this (in the push output for example) and need to get this output to reformat it, we'll need to actually create a buffer, call this, let it write, parse the buffer and then reformat it and write to stdout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about it, I think I can do something a little nicer with the other refactors I haven't pushed yet. One sec.

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.

3 participants