Please consider fixing more bad APIs in v2.0 #2902
Comments
|
Hi @majewsky, thank you for coming forward. We have chosen to take a backwards-compatible approach to most changes, including the adoption of The conversation on errors is a bit more complicated. I agree that Gophercloud errors need restructuring, and at the same time I'm afraid of breaking users in nasty ways. We didn't get complaints, so we assumed that we could just postpone the resolution of this issue. That's why I'm glad to hear from you. I would like all errors in Gophercloud to implement Note that you're welcome to join the #gophercloud channel in the Kubernetes Slack workspace. |
|
@majewsky do you have a proposal for errors? |
As I said, just get rid of is too tedious, you could do something like https://pkg.go.dev/github.com/majewsky/schwift#Is and provide a custom error matching function for matching a specific response code. |
|
I quickly wipped up a PR for the error-type thing (#2904) since that's something that I could do very quickly. Hopefully this either ends up simplifying the API, or improving clarity about why the larger API surface is actually necessary. Re "adding |
That would be ideal; writing a good tool is also contemplated within the "time-consuming effort" I mentioned above. There is no deadline for adding |
|
For the "add ctx everywhere" thing, I am about 90% done on a change that does that. Will put it up as a PR in a few minutes to solicit feedback and see if it's worth to do the remaining 10%. |
majewsky
added a commit
to sapcc/gophercloud
that referenced
this issue
Feb 9, 2024
This is not intended to be merged in this form, but serves as a demonstration to make a point. @pierreprinetti said in gophercloud#2902: > We have chosen to take a backwards-compatible approach to most changes, including the adoption of context.Context. Now that the main library is ready, we want to over time add WithContext versions of, ideally, every single call in Gophercloud. This is a time-consuming effort that you're welcome to join, and it can be done within v2 (or even v1) without a major version bump. I felt that the part about this being a time-consuming effort is a misconception, so I tried how long this actually takes. What is in this PR took about 1.5 hours to produce and is probably 80-90% of the way there in terms of actual code changes, so I think this is something that should be considered for 2.0. This obviously does not consider the time spent reviewing it, but if the aforementioned decision to introduce context-awareness slowly was taken based on the assumption that it requires a lot of person-hours that need to be spread out, I don't think that's really true. As a downstream user of this library, I have waited for a long time for it to become context-aware, then got excited seeing it on the 1.9.0 release notes, then immediately disappointed upon seeing that the API surface in 1.9.0 is not useful in practical applications. Now hearing that it will be another entire major version before I can fully convert my programs to context-aware for good is quite frustrating, especially if it could be done sooner. This is my motivation for proposing this alternate path. And besides, I cannot bear the thought of you folks possibly wasting dozens of hours on writing these changes manually in a tedious backwards-compatible manner when it could be done through scripting in a few minutes. I do not intend for this PR to be merged: - It has test failures because I only checked `go vet ./openstack/...`. Specifically, additional code changes will be required in the acceptance tests. - It is ludicrously large and flies in the face of the "small PRs" requirement in the contribution guidelines. If you guys decide that you want to move forward on this change as presented here, I can try and split it up into one PR per service and add the required changes to the acceptance tests etc. For now, this is a Request for Comments in the original sense of the term, not a literal Pull Request. --- For reproducibility, here is how I prepared this changeset. Most of the auto changes are through `sed -i`, each time followed by quick manual confirmation through `git add -p`. I also ran `goimports -w` on all the files touched by `sed -i` to add `import "context"` as needed. Then I planned the next auto change by running `go vet ./openstack/...`, picking an error at random, and generalizing it into a regex. ``` sed -i 's/\<client\.\(Get\|Post\|Put\|Patch\|Delete\|Head\|Request\)(/client.\1WithContext(ctx, /g' openstack/**/*.go sed -i '!/\<Pager\>/s/^func \(\w\+\)(\(\w\+\) \*gophercloud.ServiceClient\>/func \1(ctx context.Context, \2 *gophercloud.ServiceClient/' openstack/**/requests.go openstack/**/request.go ``` This adds the `ctx` argument to most Result-returning functions, and uses it in the `client.Get` etc. calls within. ``` sed -i 's/(\(\w\+\).ServiceClient()/(context.TODO(), \1.ServiceClient()/' openstack/**/*_test.go sed -i 's/(\(&\?\(c\|sc\|client\)\), /(context.TODO(), \1, /' openstack/**/*_test.go ``` This adds a `context.TODO()` argument to most test invocations of those functions. ``` sed -i 's/List\(\w*\)(ctx, /List\1(/' openstack/**/*.go sed -i 's/List\(\w*\)(context.TODO(), /List\1(/' openstack/**/*_test.go ``` Functions called `List...` usually return a Pager instead of a Result, so this reverts the previous changes there. At this point, `go vet ./openstack/...` was only showing a few dozen things, so I flipped through those files manually with ``` vim $(go vet ./openstack/... |& sed -n 's/^vet: \(.*\)\.go:[0-9].*/\1.go/p') ``` To clean that up, I changed: - the various `func WaitForStatus` in `openstack/**/util.go` - the various functions in `openstack/utils/choose_version.go` - some specialty interfaces that my regexes and/or fileglobs did not catch - two instances where regexes replaces a `client.Get()` that was not a ServiceClient, but a `http.Client` (reverted those changes) - about a dozen instances where Pager-returning functions had a superfluous `ctx` argument because they were not called `List...`, and Result-returning functions that did not have `ctx` because they were called `List...`
pierreprinetti
pushed a commit
to shiftstack/gophercloud
that referenced
this issue
Feb 21, 2024
This is not intended to be merged in this form, but serves as a demonstration to make a point. @pierreprinetti said in gophercloud#2902: > We have chosen to take a backwards-compatible approach to most changes, including the adoption of context.Context. Now that the main library is ready, we want to over time add WithContext versions of, ideally, every single call in Gophercloud. This is a time-consuming effort that you're welcome to join, and it can be done within v2 (or even v1) without a major version bump. I felt that the part about this being a time-consuming effort is a misconception, so I tried how long this actually takes. What is in this PR took about 1.5 hours to produce and is probably 80-90% of the way there in terms of actual code changes, so I think this is something that should be considered for 2.0. This obviously does not consider the time spent reviewing it, but if the aforementioned decision to introduce context-awareness slowly was taken based on the assumption that it requires a lot of person-hours that need to be spread out, I don't think that's really true. As a downstream user of this library, I have waited for a long time for it to become context-aware, then got excited seeing it on the 1.9.0 release notes, then immediately disappointed upon seeing that the API surface in 1.9.0 is not useful in practical applications. Now hearing that it will be another entire major version before I can fully convert my programs to context-aware for good is quite frustrating, especially if it could be done sooner. This is my motivation for proposing this alternate path. And besides, I cannot bear the thought of you folks possibly wasting dozens of hours on writing these changes manually in a tedious backwards-compatible manner when it could be done through scripting in a few minutes. I do not intend for this PR to be merged: - It has test failures because I only checked `go vet ./openstack/...`. Specifically, additional code changes will be required in the acceptance tests. - It is ludicrously large and flies in the face of the "small PRs" requirement in the contribution guidelines. If you guys decide that you want to move forward on this change as presented here, I can try and split it up into one PR per service and add the required changes to the acceptance tests etc. For now, this is a Request for Comments in the original sense of the term, not a literal Pull Request. --- For reproducibility, here is how I prepared this changeset. Most of the auto changes are through `sed -i`, each time followed by quick manual confirmation through `git add -p`. I also ran `goimports -w` on all the files touched by `sed -i` to add `import "context"` as needed. Then I planned the next auto change by running `go vet ./openstack/...`, picking an error at random, and generalizing it into a regex. ``` sed -i 's/\<client\.\(Get\|Post\|Put\|Patch\|Delete\|Head\|Request\)(/client.\1WithContext(ctx, /g' openstack/**/*.go sed -i '!/\<Pager\>/s/^func \(\w\+\)(\(\w\+\) \*gophercloud.ServiceClient\>/func \1(ctx context.Context, \2 *gophercloud.ServiceClient/' openstack/**/requests.go openstack/**/request.go ``` This adds the `ctx` argument to most Result-returning functions, and uses it in the `client.Get` etc. calls within. ``` sed -i 's/(\(\w\+\).ServiceClient()/(context.TODO(), \1.ServiceClient()/' openstack/**/*_test.go sed -i 's/(\(&\?\(c\|sc\|client\)\), /(context.TODO(), \1, /' openstack/**/*_test.go ``` This adds a `context.TODO()` argument to most test invocations of those functions. ``` sed -i 's/List\(\w*\)(ctx, /List\1(/' openstack/**/*.go sed -i 's/List\(\w*\)(context.TODO(), /List\1(/' openstack/**/*_test.go ``` Functions called `List...` usually return a Pager instead of a Result, so this reverts the previous changes there. At this point, `go vet ./openstack/...` was only showing a few dozen things, so I flipped through those files manually with ``` vim $(go vet ./openstack/... |& sed -n 's/^vet: \(.*\)\.go:[0-9].*/\1.go/p') ``` To clean that up, I changed: - the various `func WaitForStatus` in `openstack/**/util.go` - the various functions in `openstack/utils/choose_version.go` - some specialty interfaces that my regexes and/or fileglobs did not catch - two instances where regexes replaces a `client.Get()` that was not a ServiceClient, but a `http.Client` (reverted those changes) - about a dozen instances where Pager-returning functions had a superfluous `ctx` argument because they were not called `List...`, and Result-returning functions that did not have `ctx` because they were called `List...`
pierreprinetti
pushed a commit
to shiftstack/gophercloud
that referenced
this issue
Feb 21, 2024
This is not intended to be merged in this form, but serves as a demonstration to make a point. @pierreprinetti said in gophercloud#2902: > We have chosen to take a backwards-compatible approach to most changes, including the adoption of context.Context. Now that the main library is ready, we want to over time add WithContext versions of, ideally, every single call in Gophercloud. This is a time-consuming effort that you're welcome to join, and it can be done within v2 (or even v1) without a major version bump. I felt that the part about this being a time-consuming effort is a misconception, so I tried how long this actually takes. What is in this PR took about 1.5 hours to produce and is probably 80-90% of the way there in terms of actual code changes, so I think this is something that should be considered for 2.0. This obviously does not consider the time spent reviewing it, but if the aforementioned decision to introduce context-awareness slowly was taken based on the assumption that it requires a lot of person-hours that need to be spread out, I don't think that's really true. As a downstream user of this library, I have waited for a long time for it to become context-aware, then got excited seeing it on the 1.9.0 release notes, then immediately disappointed upon seeing that the API surface in 1.9.0 is not useful in practical applications. Now hearing that it will be another entire major version before I can fully convert my programs to context-aware for good is quite frustrating, especially if it could be done sooner. This is my motivation for proposing this alternate path. And besides, I cannot bear the thought of you folks possibly wasting dozens of hours on writing these changes manually in a tedious backwards-compatible manner when it could be done through scripting in a few minutes. I do not intend for this PR to be merged: - It has test failures because I only checked `go vet ./openstack/...`. Specifically, additional code changes will be required in the acceptance tests. - It is ludicrously large and flies in the face of the "small PRs" requirement in the contribution guidelines. If you guys decide that you want to move forward on this change as presented here, I can try and split it up into one PR per service and add the required changes to the acceptance tests etc. For now, this is a Request for Comments in the original sense of the term, not a literal Pull Request. --- For reproducibility, here is how I prepared this changeset. Most of the auto changes are through `sed -i`, each time followed by quick manual confirmation through `git add -p`. I also ran `goimports -w` on all the files touched by `sed -i` to add `import "context"` as needed. Then I planned the next auto change by running `go vet ./openstack/...`, picking an error at random, and generalizing it into a regex. ``` sed -i 's/\<client\.\(Get\|Post\|Put\|Patch\|Delete\|Head\|Request\)(/client.\1WithContext(ctx, /g' openstack/**/*.go sed -i '!/\<Pager\>/s/^func \(\w\+\)(\(\w\+\) \*gophercloud.ServiceClient\>/func \1(ctx context.Context, \2 *gophercloud.ServiceClient/' openstack/**/requests.go openstack/**/request.go ``` This adds the `ctx` argument to most Result-returning functions, and uses it in the `client.Get` etc. calls within. ``` sed -i 's/(\(\w\+\).ServiceClient()/(context.TODO(), \1.ServiceClient()/' openstack/**/*_test.go sed -i 's/(\(&\?\(c\|sc\|client\)\), /(context.TODO(), \1, /' openstack/**/*_test.go ``` This adds a `context.TODO()` argument to most test invocations of those functions. ``` sed -i 's/List\(\w*\)(ctx, /List\1(/' openstack/**/*.go sed -i 's/List\(\w*\)(context.TODO(), /List\1(/' openstack/**/*_test.go ``` Functions called `List...` usually return a Pager instead of a Result, so this reverts the previous changes there. At this point, `go vet ./openstack/...` was only showing a few dozen things, so I flipped through those files manually with ``` vim $(go vet ./openstack/... |& sed -n 's/^vet: \(.*\)\.go:[0-9].*/\1.go/p') ``` To clean that up, I changed: - the various `func WaitForStatus` in `openstack/**/util.go` - the various functions in `openstack/utils/choose_version.go` - some specialty interfaces that my regexes and/or fileglobs did not catch - two instances where regexes replaces a `client.Get()` that was not a ServiceClient, but a `http.Client` (reverted those changes) - about a dozen instances where Pager-returning functions had a superfluous `ctx` argument because they were not called `List...`, and Result-returning functions that did not have `ctx` because they were called `List...`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I saw that you put out a v2.0.0 beta, and the breaking changes are very tame. Since this is a unique opportunity, I wonder how many thoughts have been spent on improving the core APIs. Specifically, I have two requests:
Please consider removing all the
Err4xxerinterfaces andErrDefault4xxstructs without replacement, and just returningErrUnexpectedResponseCodeinstead. Or at the very least, these types should be documented much better to explain what their purpose is. As far as I can tell, all they do is ruin good error messages unless you know how to get rid of them.The
context.Contextsupport added in the previous release is pretty much worthless since all the structured API methods do not takectxarguments. This is the best possible moment to fix that, and add actxargument to every method that wrapsProviderClient.RequestorServiceClient.Request. The variants of those methods withoutctxarguments should be removed.This just off the top of my head, there's probably more. Please consider using this opportunity if at all possible.
The text was updated successfully, but these errors were encountered: