This repository has been archived by the owner on Sep 9, 2020. It is now read-only.
[WIP] minimal module awareness #1963
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Joshua Rubin <[email protected]>
|
Right now, Perhaps as an artifact of this, pruning unused packages causes the entire package, e.g. |
joshuarubin
changed the title
Signed-off-by: Joshua Rubin <[email protected]>
Signed-off-by: Joshua Rubin <[email protected]>
|
Hi @sdboyer, I'm having a hard time tracing how things go from |
Signed-off-by: Joshua Rubin <[email protected]>
|
I've gotten the vendor directory to be constructed properly. However, it isn't particularly elegant. After exporting the revision from the source, I simply move the directories as required based on the results of |
|
@joshuarubin sorry, busy week. re: this:
i meant that in reference to how the solver itself works. That'd be a check made in this vicinity. Probably the easiest thing to do is duplicate the Re: exporting, i'd say that all the logic in there should just be updated to be module-sensitive - but, because we only operate at the whole-repo level, yes, it will probably entail some hacky moving-around-of-things after the full tree write is complete. That's fine, though - it looks like you're on the right path with where you've put those Other questions? |
olivere
added a commit
to olivere/elastic-with-go-modules
that referenced
this pull request
Aug 4, 2018
This commit updates the README to include Go 1.11 beta 3. We also add a few notes about "minimal module awareness" that has been included in Go 1.10.3 and 1.9.7. Unfortunately, the `dep` tool is still on its way to implement that. * [Minimal module awareness in Go 1.10.3 and Go 1.9.7](https://go.googlesource.com/go/+/d4e21288e444d3ffd30d1a0737f15ea3fc3b8ad9) * [Tracking implementation of "minimal module awareness" in dep](golang/dep#1963)
|
@sdboyer the gps solver works with modules already in this PR. Is there something you think should be done differently? The exporter also seems to work well in most cases already, so I'd really appreciate any sort of review on that as well. The one case where things don't seem to work, and I haven't tracked down, is when changing a constraint to a new major version (e.g. package |
|
Any chance we could keep this moving forward? |
|
ill find some time tomorrow to look again.
…On September 4, 2018 11:22:43 AM EDT, Joshua Rubin ***@***.***> wrote:
@sdboyer? @jmank88?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1963 (comment)
|
|
I'll find time to catch up on this tomorrow.
…On September 4, 2018 11:22:43 AM EDT, Joshua Rubin ***@***.***> wrote:
@sdboyer? @jmank88?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1963 (comment)
|
|
Bueller? |
|
It would be awesome to get some movement on this |
|
I'm going to try for one more release before the baby arrives, and I plan to include this
…On October 3, 2018 1:52:38 AM EDT, Joshua Rubin ***@***.***> wrote:
It would be awesome to get some movement on this
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1963 (comment)
|
|
Just another reminder to see how things are going with this |
brandur-stripe
pushed a commit
to stripe/stripe-go
that referenced
this pull request
Jan 16, 2019
Reverts: #712 (And an earlier PR that added a `go.mod` file in the first place.) Unfortunately, there are still some fairly bad incompatibilities between Go modules and Dep for users still on the latter. For now, we're going to revert Go module support because there doesn't seem to be any easy way (i.e., anything that's not maintaining a fork) of doing a good job of both. Projects can still use the Go module system, but will have to pull this package in as a pre-module incompatible one. We are hoping to eventually see basic module awareness merged into Dep: golang/dep#1963 If/when that gets done, we'll revert this revert after a small grace period, and hopefully be back to a place where both packaging systems are well supported. I'll also modify the README to clarify the current situation in another follow up PR.
brandur-stripe
pushed a commit
to stripe/stripe-go
that referenced
this pull request
Jan 16, 2019
Reverts: #712 (And an earlier PR that added a `go.mod` file in the first place.) Unfortunately, there are still some fairly bad incompatibilities between Go modules and Dep for users still on the latter. For now, we're going to revert Go module support because there doesn't seem to be any easy way (i.e., anything that's not maintaining a fork) of doing a good job of both. Projects can still use the Go module system, but will have to pull this package in as a pre-module incompatible one. We are hoping to eventually see basic module awareness merged into Dep: golang/dep#1963 If/when that gets done, we'll revert this revert after a small grace period, and hopefully be back to a place where both packaging systems are well supported. I'll also modify the README to clarify the current situation in another follow up PR.
|
Get your employer to sponsor the project, or try out the fork and report back about whether it works, both would help a lot!
Kevin Burke
phone: 925-271-7005 | kevin.burke.dev ( https://kevin.burke.dev )
…On Wed, Jun 26, 2019 at 7:33 PM, Ben Bytheway < ***@***.*** > wrote:
Having minimal module support (especially for major version import paths)
would go a long way toward easing the pain of supporting both dep and
modules. I'm only a solo developer, but is there any way folks like us can
help getting this in dep? What can we do?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#1963?email_source=notifications&email_token=AABZEI46MY5LNVH3F5MPSZ3P4QRI3A5CNFSM4FMMC7T2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYVPNUY#issuecomment-506132179
) , or mute the thread (
https://github.com/notifications/unsubscribe-auth/AABZEI4WCHTNA5Z74XWH7BDP4QRI3ANCNFSM4FMMC7TQ
).
|
|
I have pulled down this patch and given it try. It seems to do the trick for me, enabling support especially of go module libraries with v2 import paths. I've written some limited gps tests and an init integration test, which extends the commits in this repo. https://github.com/bytheway/dep/tree/module-tests. I'm happy to make another PR based on that fork-of-a-fork if that would make sense. |
|
Any change to move it forward anytime soon? It keep getting more annoying because more and more packages not using go modules. |
|
Could this change be moved forward? |
|
Building a custom version of dep based on this PR allowed me to successfully use a go module based project with dep. 👍 |
markmssd
approved these changes
Sep 22, 2019
3 tasks
|
We use this, running perfect! |
harsimranmaan
approved these changes
Oct 30, 2019
sseago
approved these changes
Dec 6, 2019
sseago
added a commit
to sseago/openshift-migration-plugin
that referenced
this pull request
Dec 6, 2019
containers/image now uses go modules, so for 'dep ensure' to work with the updated code, this upstream dep PR is needed: golang/dep#1963 Without this PR, dep does not work with dependencies which use go modules.
|
Unfortunately, @sdboyer and I have not been active on this project for some time. |
brandur-stripe
pushed a commit
to stripe/stripe-go
that referenced
this pull request
Apr 10, 2020
Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963
remi-stripe
pushed a commit
to stripe/stripe-go
that referenced
this pull request
Apr 16, 2020
Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963
remi-stripe
pushed a commit
to stripe/stripe-go
that referenced
this pull request
Apr 16, 2020
Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963
brandur-stripe
added a commit
to stripe/stripe-go
that referenced
this pull request
Apr 17, 2020
* Make API response accessible on returned API structs (#1054) * Make API response accessible on returned API structs Makes an API response struct containing niceties like the raw response body, status, and request ID accessible via API resource structs returned from client functions. For example: customer, err := customer.New(params) fmt.Printf("request ID = %s\n", customer.LastResponse.RequestID) This is a feature that already exists in other language API libraries and which is requested occasionally here, usually for various situations involving more complex usage or desire for better observability. -- Implementation We introduce a few new types to make this work: * `APIResponse`: Represents a response from the Stripe API and includes things like request ID, status, and headers. I elected to create my own object instead of reusing `http.Response` because it gives us a little more flexibility, and hides many of myriad of fields exposed by the `http` version, which will hopefully give us a little more API stability/forward compatibility. * `APIResource`: A struct that contains `LastResponse` and is meant to represent any type that can we returned from a Stripe API endpoint. A coupon is an `APIResource` and so is a list object. This struct is embedded in response structs where appropriate across the whole API surface area (e.g. `Coupon`, `ListMeta`, etc.). * `LastResponseGetter`: A very basic interface to an object that looks like an `APIResource`. This isn't strictly necessary, but gives us slightly more flexibility around the API and makes backward compatibility a little bit better for non-standard use cases (see the section on that below). `stripe.Do` and other backend calls all start taking objects which are `LastResponseGetter` instead of `interface{}`. This provides us with some type safety around forgetting to include an embedded `APIResource` on structs that should have it by making the compiler balk. As `stripe.Do` finishes running a request, it generates an `APIResponse` object and sets it onto the API resource type it's deserializing and returning (e.g. a `Coupon`). Errors also embed `APIResource` and similarly get access to the same set of fields as response resources, although in their case some of the fields provided in `APIResponse` are duplicates of what they had already (see "Caveats" below). -- Backwards compatibility This is a minor breaking change in that backend implementations methods like `Do` now take `LastResponseGetter` instead of `interface{}`, which is more strict. The good news though is that: * Very few users should be using any of these even if they're technically public. The resource-specific clients packages tend to do all the work. * Users who are broken should have a very easy time updating code. Mostly this will just involve adding `APIResource` to structs that were being passed in. -- Naming * `APIResponse`: Went with this instead of `StripeResponse` as we see in some other libraries because the linter will complain that it "stutters" when used outside of the package (meaning, uses the same word twice in a row), i.e. `stripe.StripeResponse`. `APIResponse` sorts nicely with `APIResource` though, so I think it's okay. * `LastResponse`: Copied the "last" convention from other API libraries like stripe-python. * `LastResponseGetter`: Given an "-er" name per Go convention around small interfaces that are basically one liners -- e.g. `Reader`, `Writer, `Formatter`, `CloseNotifier`, etc. I can see the argument that this maybe should just be `APIResourceInterface` or something like that in case we start adding new things, but I figure at that point we can either rename it, or create a parent interface that encapsulates it: ``` go type APIResourceInterface interface { LastResponseGetter } ``` -- Caveats * We only set the last response for top-level returned objects. For example, an `InvoiceItem` is an API resource, but if it's returned under an `Invoice`, only `Invoice` has a non-nil `LastResponse`. The same applies for all resources under list objects. I figure that doing it this way is more performant and makes a little bit more intuitive sense. Users should be able to work around it if they need to. * There is some duplication between `LastResponse` and some other fields that already existed on `stripe.Error` because the latter was already exposing some of this information, e.g. `RequestID`. I figure this is okay: it's nice that `stripe.Error` is a `LastResponseGetter` for consistency with other API resources. The duplication is a little unfortunate, but not that big of a deal. * Rename `LastResponseGetter` to `LastResponseSetter` and remove a function * Update stripe.go Co-Authored-By: Olivier Bellone <[email protected]> * Move `APIResource` onto individual list structs instead of having it in `ListMeta` Co-authored-by: Brandur <[email protected]> Co-authored-by: Olivier Bellone <[email protected]> * Remove all beta features from Issuing APIs * Multiple breaking API changes * `PaymentIntent` is now expandable on `Charge` * `Percentage` was removed as a filter when listing `TaxRate` * Removed `RenewalInterval` on `SubscriptionSchedule` * Removed `Country` and `RoutingNumber` from `ChargePaymentMethodDetailsAcssDebit` * Start using Go Modules Similar to the original implementation for Go Modules in #712, here we add a `go.mod` and `go.sum`, then proceed to use Go Modules style import paths everywhere that include the current major revision. Unfortunately, this may still cause trouble for Dep users who are trying to upgrade stripe-go, but the project's now had two years to help its users with basic Go Module awareness, and has chosen not to do so. It's received no commits of any kind since August 2019, and therefore would be considered unmaintained by most definitions. Elsewhere, Go Modules now seem to be the only and obvious way forward, so we're likely to see more and more users on them. `scripts/check_api_clients/main.go` is also updated to be smarter about breaking down package paths which may now include the major. [1] golang/dep#1963 * Change v71 back to v70 Move back down to current major version so that we can test that our release script will bump it to v71 properly when the time comes. Co-authored-by: Brandur <[email protected]> Co-authored-by: Olivier Bellone <[email protected]> Co-authored-by: Remi Jannel <[email protected]>
|
Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
go.modfilevendor/directory is populated correctly (importx/y/v2/fooshould not be stored asvendor/x/y/foo)CHANGELOG.mdWhat does this do / why do we need it?
Adds minimal module awareness, as Go 1.10.3 did.
Which issue(s) does this PR fix?
Fixes #1962