Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

tidy: convert Fprintf to Fprintln when possible #7174

Conversation

rvantonder
Copy link
Contributor

When the second argument to Fprintf contains no format specifiers (AKA "verbs"), use Fprint instead.

In our codebase, every pattern that matched the above also contains a \n at the end of the string, so we can further simplify the above and say that:

  • if the string also ends with \n, use Fprintln instead.

Created with:

{
    "scopeQuery": "repo:github.com/sourcegraph/sourcegraph$",
    "matchTemplate":   "fmt.Fprintf(:[1], \":[2]\\n\")",
    "rewriteTemplate": "fmt.Fprintln(:[1], \":[2]\")"
}

@rvantonder rvantonder requested a review from a team December 12, 2019 18:15
@rvantonder rvantonder requested a review from a team December 12, 2019 18:15
@rvantonder
Copy link
Contributor Author

Haha, oh, the test build failure is interesting:

Screen Shot 2019-12-12 at 11 24 35 AM

That's a false positive in the linter, it's not redundant, because the original code does want to print two newlines.

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

How cool is that?!

@rvantonder
Copy link
Contributor Author

@mrnugget apparently there's a thread on this: golang/go#18085 (comment) (creds @slimsag for hunting down the issue)

The `Fprintln("...\n")` triggers golang/go#18085 (comment), so using `Fprint("...\n")` instead. I'm changing it for both `Fprintf` calls in this block, even though the one can be `Fprintln` and the other `Fprint`, so that it's consistent and we don't see a weird mix of `Fprintln` and `Fprintf` here.
@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #7174 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master    #7174   +/-   ##
=======================================
  Coverage   39.19%   39.19%           
=======================================
  Files        1230     1230           
  Lines       64159    64159           
  Branches     6066     6066           
=======================================
  Hits        25150    25150           
  Misses      36709    36709           
  Partials     2300     2300
Impacted Files Coverage Δ
cmd/server/shared/postgres.go 0% <0%> (ø) ⬆️

@rvantonder rvantonder merged commit d03b895 into master Dec 12, 2019
@rvantonder rvantonder deleted the sourcegraph/tidy-convert-fprintf-to-fprintln-when-possible-1576174509 branch December 12, 2019 19:46
@uwedeportivo
Copy link
Contributor

i'm late to the party, but this is really cool

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants