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

moving globalApiPrefix to Client #83

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

luthermonson
Copy link

New attempt with all latest changes from gomod included

Closes #80

@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #83 into master will decrease coverage by 0.5%.
The diff coverage is 97.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage     100%   99.49%   -0.51%     
==========================================
  Files          31       31              
  Lines        1173     1187      +14     
==========================================
+ Hits         1173     1181       +8     
- Misses          0        3       +3     
- Partials        0        3       +3
Impacted Files Coverage Δ
draft_order.go 100% <100%> (ø) ⬆️
customer_address.go 100% <100%> (ø) ⬆️
redirect.go 100% <100%> (ø) ⬆️
util.go 100% <100%> (ø) ⬆️
applicationcharge.go 100% <100%> (ø) ⬆️
inventory_item.go 100% <100%> (ø) ⬆️
usagecharge.go 100% <100%> (ø) ⬆️
product.go 100% <100%> (ø) ⬆️
collect.go 100% <100%> (ø) ⬆️
image.go 100% <100%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 054a36d...b0fbcb3. Read the comment docs.

@rickywiens
Copy link

Hey @luthermonson,

Just wondering if we can cover some of the new cases you added in the goshopify.go file?

https://codecov.io/gh/bold-commerce/go-shopify/pull/83/src/goshopify.go

I'm a bit less concerned about the oauth.go changes, like you mentioned the newClient change kinda messed that up and we can clean up those tests ourselves later.

@luthermonson
Copy link
Author

@rickywiens ya im working on it just havent found time yet! didn't think this was gonna get merged till i fixed those but had other stuff come up. adding Client as a field on App was the hack to get around it but i think that GetAccessToken method should be on Client personally.

@luthermonson
Copy link
Author

@rickywiens is this good enough or more done with oauth?

@rickywiens
Copy link

@rickywiens is this good enough or more done with oauth?

I think this should be good - we can patch up the tests later.

It was brought up internally that we might want to alter the import in the gomod file:

https://github.com/golang/go/wiki/Modules#semantic-import-versioning

What do you think about that?

@rickywiens rickywiens requested a review from gordcurrie February 3, 2020 22:09
oauth.go Show resolved Hide resolved
goshopify.go Show resolved Hide resolved
@luthermonson
Copy link
Author

@rickywiens is this good enough or more done with oauth?

I think this should be good - we can patch up the tests later.

It was brought up internally that we might want to alter the import in the gomod file:

https://github.com/golang/go/wiki/Modules#semantic-import-versioning

What do you think about that?

I think you mean changing this to module github.com/bold-commerce/go-shopify/v3 at the top of go.mod? I'm against this, but it's your project. Adding the go.mod file didn't change the API of the package, just let the gomod nerds use it properly.

@luthermonson
Copy link
Author

@rickywiens needed anything else for this?

@rickywiens
Copy link

Hey @luthermonson. I was just trying to figure out how to get around the codecov requirement. I'm merging it locally now and I will push it up.

@rickywiens rickywiens merged commit af3a6fd into bold-commerce:master Feb 6, 2020
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.

Can't Use Multiple Versioned API Clients
3 participants