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

Pagination to return interface not concrete type #328

Merged
merged 4 commits into from Jan 13, 2022

Conversation

Integralist
Copy link
Collaborator

The recent pagination implementations were designed to return concrete types, which include methods attached to the returned type that in some cases are responsible for making API calls.

In the Fastly CLI we're unable to mock these returned concrete types and so in this PR I've swapped them out for interfaces.

NOTE: We can reduce the boilerplate once go 1.18 comes out and we have access to generics.

This is a breaking change and will require a new major version of the go-fastly client.

Copy link
Contributor

@smaeda-ks smaeda-ks left a comment

Choose a reason for hiding this comment

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

LGTM, except for some naming convention consistency suggestions I just left. And if this is going to be a major version release it's probably better to include #325 as well.

fastly/dictionary_item.go Outdated Show resolved Hide resolved
fastly/dictionary_item_test.go Outdated Show resolved Hide resolved
fastly/paginator.go Outdated Show resolved Hide resolved
Integralist and others added 3 commits January 13, 2022 09:10
Co-authored-by: Shohei Maeda <irt_m.jrsyo@ntworkers.com>
Co-authored-by: Shohei Maeda <irt_m.jrsyo@ntworkers.com>
Co-authored-by: Shohei Maeda <irt_m.jrsyo@ntworkers.com>
@Integralist
Copy link
Collaborator Author

@smaeda-ks OK cool thanks. I'll merge this and I'll look at #325 (do you know if that's almost ready for a review)?

@smaeda-ks
Copy link
Contributor

@Integralist As for #325, note quite yet. Due to how go-fastly runs tests using recorded fixtures, I'm still not 100% confident to say it's okay to merge as it's not making real requests each time. So my idea is to run TF provider full tests using that branch and see if anything comes up. Since our provider is a large consumer of this library we could rely on that result relatively confidently. Thoughts?

@Integralist
Copy link
Collaborator Author

Integralist commented Jan 13, 2022

@smaeda-ks SGTM 👍🏻 OK let me know when it's ready for a review, and/or if you need any help.

@Integralist Integralist merged commit e452671 into main Jan 13, 2022
@Integralist Integralist deleted the integralist/pagination-interface branch January 13, 2022 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants