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

Fix duplicate error message #1144

Merged
merged 3 commits into from
May 7, 2023
Merged

Fix duplicate error message #1144

merged 3 commits into from
May 7, 2023

Conversation

pd93
Copy link
Member

@pd93 pd93 commented Apr 27, 2023

Fixes #1141

image

I think this PR is a nice, clean solution to the problem. Unfortunately, as you can see, it also has the side-effect of removing the red coloured text from the error message since the executor/logger are out of scope in main 🙁 I'm not sure what the best solution is for this. We weren't always consistent with showing errors in coloured text anyway.

  • We could potentially return the executor/logger back to the main function and use the logger that way. However, the logger isn't actually setup until we call e.Setup() and errors can occur before that which would result in nil pointer panics.
  • We could just not use the logger at all and print a raw ANSI code to get the color, but we'd need to build in cases for the --color flag and NO_COLOR envvar.

@andreynering wdyt?

Also, since we needed access to the flags in the main function, I've moved them to a package-level struct variable. It means we can access them outside the run() function and also has the nice side-effect of namespacing the variables. i.e. versionFlag -> flag.version

@pd93 pd93 added the type: bug Something not working as intended. label May 6, 2023
Using the logger package so envs like NO_COLOR and FORCE_COLOR keeps working.
@andreynering
Copy link
Member

Hey @pd93, thanks for the fix.

Take a look at the commit. I just instantiated the logger.Logger struct manually and it was enough to make it work and still respect settings like --color=false, NO_COLOR=1, etc.

@andreynering andreynering merged commit 38341ff into main May 7, 2023
@andreynering andreynering deleted the fix-duplicate-error-message branch May 7, 2023 00:11
andreynering added a commit that referenced this pull request May 7, 2023
@pd93 pd93 mentioned this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something not working as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

double error output
2 participants