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: suppress debug logs when using -q #1254

Merged
merged 2 commits into from Jul 19, 2022

Conversation

Jake-Convictional
Copy link
Contributor

@Jake-Convictional Jake-Convictional commented Jul 6, 2022

Describe the PR
Fixes an issue where certain debug logs were always directed to stderr, even if the -q flag was provided.

Relation issue
#1255

Additional context
This fix will be helpful for anyone who wants to suppress all debug output (e.g. when generating API docs as part of a git commit hook).

I took the path of least resistance here since I'm not sure on the development philosophy for this project. For example, I initialized the new Gen.debug field in the Build() method instead of the New() method. Happy to pass the debugger in as an arg to New() instead if that's preferred.

I did run the changes and they work as expected.

var logger swag.Debugger
logger := log.New(os.Stdout, "", log.LstdFlags)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving initializing the debugger to here to avoid nil confusions.

gen/gen.go Outdated
@@ -117,6 +123,7 @@ type Config struct {

// Build builds swagger json file for given searchDir and mainAPIFile. Returns json.
func (g *Gen) Build(config *Config) error {
g.debug = config.Debugger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would make more sense to initialize this in the New() method. But since there are no other args being passed in there yet, I hesitated.

Copy link
Contributor

@ubogdan ubogdan Jul 13, 2022

Choose a reason for hiding this comment

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

debug.New() should init Gen struct with debug field of value log.New(os.Stdout, "", log.LstdFlags) so the old behavior wont break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ubogdan! Fixed it now, looks like the tests are passing again.

Comment on lines -141 to +148
log.Printf("Using overrides from %s", config.OverridesFile)
g.debug.Printf("Using overrides from %s", config.OverridesFile)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

log.Printf outputs to stderr, whereas g.debug.Printf will output either to stdout or be discarded.

@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #1254 (312f1bc) into master (bd21bb0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1254   +/-   ##
=======================================
  Coverage   94.99%   95.00%           
=======================================
  Files          14       14           
  Lines        2618     2620    +2     
=======================================
+ Hits         2487     2489    +2     
  Misses         72       72           
  Partials       59       59           
Impacted Files Coverage Δ
gen/gen.go 93.42% <100.00%> (+0.06%) ⬆️
parser.go 93.59% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd21bb0...312f1bc. Read the comment docs.

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

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

LGTM

@ubogdan ubogdan merged commit 89c61d4 into swaggo:master Jul 19, 2022
@Jake-Convictional
Copy link
Contributor Author

(Whoops, hadn't noticed this was already merged when I requested review 😄, thanks! )

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.

None yet

2 participants