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 the same output for surveys as for logs. #884

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

jkinkead
Copy link
Contributor

@jkinkead jkinkead commented Sep 5, 2022

Add a package variable to hold the survey output writer and a setter for it.

Add a helper to generate a survey option using the writer for stdout.

Initialize the survey output to be the same as for the log module.


Fixes #883 .

Add a package variable to hold the survey output writer and a setter for it.

Add a helper to generate a survey option using the writer for stdout.

Initialize the survey output to be the same as for the log module.
@jkinkead
Copy link
Contributor Author

jkinkead commented Sep 7, 2022

One thing I forgot to note: The survey library can't just query log.Writer because it requires a special type that include a file descriptor, and there is no way to get an fd from log.

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #884 (959bc67) into master (7cc41b5) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #884      +/-   ##
==========================================
- Coverage   39.11%   39.09%   -0.02%     
==========================================
  Files          53       53              
  Lines        7973     7977       +4     
==========================================
  Hits         3119     3119              
- Misses       4440     4444       +4     
  Partials      414      414              
Flag Coverage Δ
unittests 39.09% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/prompter/survey.go 3.12% <0.00%> (-0.21%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

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

@jkinkead Merging this will bump our test coverage down. Could you please consider adding tests?

Copy link
Contributor

@mapkon mapkon left a comment

Choose a reason for hiding this comment

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

Any chance this can be worked on? We need parity with the current test coverage to merge

@mapkon
Copy link
Contributor

mapkon commented Jun 24, 2023

Any chance this can be worked on? We need parity with the current test coverage to merge

@jkinkead

Copy link

@malept malept left a comment

Choose a reason for hiding this comment

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

Merging this will bump our test coverage down. Could you please consider adding tests?

I think that any tests that could be added in this PR would be primarily performative, instead of, for example, being used to spot regressions in future PRs. From what I can tell, the survey module is not conducive to test writing to begin with.

)

// outputWriter is where for all prompts will be printed. Defaults to os.Stder.
Copy link

Choose a reason for hiding this comment

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

Nit: typo

Suggested change
// outputWriter is where for all prompts will be printed. Defaults to os.Stder.
// outputWriter is where for all prompts will be printed. Defaults to os.Stderr.

@mapkon mapkon merged commit 4ac5dca into Versent:master Nov 1, 2023
8 checks passed
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.

Prompt output always goes to stdout instead of to the log stream
4 participants