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

x/sync/singleflight/v2: add package #23

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmhodges
Copy link

This adds a new package, x/sync/singleflight/v2, which is a version of
x/sync/singleflight that uses generics instead of string, and
interface{} in its API.

The biggest changes are to the Do methods, and the Result type. The
Do methods now take a comparable generic type instead of string
for its inputs (matching that of map and similar). The output type of
the Do method and the Val field in Result are now types that user
can specify instead interface{}.

Along the way, some tests received modifications to remove some
now-unneeded fmt.Sprintf calls or add an empty struct for nil return
tests.

Also, ExampleGroup also received some additions to make it clear that
non-string input types are acceptable.

This is following a similar pattern as discussed with the math/rand/v2
project. There is, however, one difference in affordances between
packages in the stdlib and outside of the stdlib that we try to
accomadate here.

Stdlib packages like math/rand/v2 can rely on the Go compiler version
being the one our package is released with and, for packages outside of
the stdlib, the errors can sometimes be cryptic when the package needs a
more modern Go compile than a user attempted to use.

For instance, singleflight/v2 has a build tag specifying the need for
Go 1.18 or later to be compiled in its necessary code files. When an
older compiler is used to build it, the user will get an error starting
with "build constraints exclude all Go files in". This makes sense when
you read all of the files, but a user may be using it as a transitive
dependency and won't know whether the build tags aren't matching because
their OS is unsupported or their CPU architecture or what.

To ameliorate user confusion, an extra notgo18.go file has been added
with build tags saying that it's only built if the Go compiler isn't
version 1.18 or later. And that file has a compile error in it that will
error like so:

$  go1.17.2 build .
# golang.org/x/sync/singleflight/v2
./notgo18.go:16:25: undefined: REQUIRES_GO_1_18_OR_LATER

That error message is an attempt to be more clear to users about what
needs to change in their build.

Another alternative to that would be to change x/sync's go.mod to
require Go 1.18 or greater. However, the rest of the x/sync packages
don't require Go 1.18, and that seems like too large of a breaking
change for singleflight/v2 alone.

This adds a new package, x/sync/singleflight/v2, which is a version of
x/sync/singleflight that uses generics instead of `string`, and
`interface{}` in its API.

The biggest changes are to the `Do` methods, and the `Result` type. The
`Do` methods now take a `comparable` generic type instead of `string`
for its inputs (matching that of `map` and similar). The output type of
the `Do` method and the `Val` field in `Result` are now types that user
can specify instead `interface{}`.

Along the way, some tests received modifications to remove some
now-unneeded `fmt.Sprintf` calls or add an `empty` struct for nil return
tests.

Also, `ExampleGroup` also received some additions to make it clear that
non-`string` input types are acceptable.

This is following a similar pattern as discussed with the `math/rand/v2`
project. There is, however, one difference in affordances between
packages in the stdlib and outside of the stdlib that we try to
accomadate here.

Stdlib packages like `math/rand/v2` can rely on the Go compiler version
being the one our package is released with and, for packages outside of
the stdlib, the errors can sometimes be cryptic when the package needs a
more modern Go compile than a user attempted to use.

For instance, `singleflight/v2` has a build tag specifying the need for
Go 1.18 or later to be compiled in its necessary code files. When an
older compiler is used to build it, the user will get an error starting
with "build constraints exclude all Go files in". This makes sense when
you read all of the files, but a user may be using it as a transitive
dependency and won't know whether the build tags aren't matching because
their OS is unsupported or their CPU architecture or what.

To ameliorate user confusion, an extra `notgo18.go` file has been added
with build tags saying that it's only built if the Go compiler isn't
version 1.18 or later. And that file has a compile error in it that will
error like so:

    $  go1.17.2 build .
    # golang.org/x/sync/singleflight/v2
    ./notgo18.go:16:25: undefined: REQUIRES_GO_1_18_OR_LATER

That error message is an attempt to be more clear to users about what
needs to change in their build.

Another alternative to that would be to change x/sync's `go.mod` to
require Go 1.18 or greater. However, the rest of the x/sync packages
don't require Go 1.18, and that seems like too large of a breaking
change for singleflight/v2 alone.
@gopherbot
Copy link

This PR (HEAD: b9dcbe5) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/sync/+/524955.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to register for Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/524955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from tricium-prod@appspot.gserviceaccount.com:

Patch Set 1:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/524955.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Jeff Hodges:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/524955.
After addressing review feedback, remember to publish your drafts!

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.

2 participants