Skip to content

cli/streams: minor refactoring and docs touch-ups#4153

Merged
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:streams_cleanup
Apr 3, 2023
Merged

cli/streams: minor refactoring and docs touch-ups#4153
thaJeztah merged 1 commit intodocker:masterfrom
thaJeztah:streams_cleanup

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

  • Touch-up GoDoc to better document each method, adding punctuation, and use doc-links where applicable.
  • SetRawTerminal(): change the order in which we check if a terminal is connected; check the local boolean first before checking if the NORAW env-var is set.
  • NewOut() / NewIn(); remove intermediate variables
  • Remove explicit use of the embedded "commonStream" to make the code slightly less verbose, and more "to the point".
  • Document the intended purpose of SetIsTerminal(), which was added in moby/moby@b2551c6 (Unit tests for cli/command/image package moby/moby#32248) to be used in unit-tests.

- Description for the changelog

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

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Apr 1, 2023
@thaJeztah thaJeztah added this to the 24.0.0 milestone Apr 1, 2023
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #4153 (555c8f4) into master (a0756c3) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4153   +/-   ##
=======================================
  Coverage   59.03%   59.03%           
=======================================
  Files         287      287           
  Lines       24767    24767           
=======================================
  Hits        14620    14620           
  Misses       9264     9264           
  Partials      883      883           

Comment thread cli/streams/out.go Outdated
ws, err := term.GetWinsize(o.fd)
if err != nil {
logrus.Debugf("Error getting size: %s", err)
logrus.WithError(err).Debugf("Error getting TTY size")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: s/Debugf/Debug/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

doh! good catch. Let me update

- Touch-up GoDoc to better document each method, adding punctuation, and
  use doc-links where applicable.
- SetRawTerminal(): change the order in which we check if a terminal is
  connected; check the local boolean first before checking if the NORAW
  env-var is set.
- NewOut() / NewIn(); remove intermediate variables
- Remove explicit use of the embedded "commonStream" to make the code
  slightly less verbose, and more "to the point".
- Document the intended purpose of SetIsTerminal(), which was added in
  moby/moby@b2551c6
  to be used in unit-tests.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah merged commit a5d4fb1 into docker:master Apr 3, 2023
@thaJeztah thaJeztah deleted the streams_cleanup branch April 3, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants