-
Notifications
You must be signed in to change notification settings - Fork 387
Conversation
4ddf98d
to
eeb940e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good overall! Had a couple of minor comments but the logic looks solid to me.
@paulomarg Any thoughts on the location of this code within the project? I think it would make a lot of sense to be able to call this off of |
Hm, that's an interesting question - right now I see a bit of tension between |
eeb940e
to
9a6ffd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Looking for the specific errors in the test makes it much easier to see what they are actually testing too, which is a nice bonus.
9a6ffd1
to
cbdb1ca
Compare
WHAT is this pull request doing?
Adds
withSession
util/helper. This allows the user to pass in a set of parameters and receive back a client that is already hooked into the current active session (or the requested offline session), as well as the session itself.For review, I would love extra eyes on my testing setup (all lines are covered but is there anything I should change or improve?).
Additionally, wondering if we should move this into the
clients
directory, as I think it would make sense to be able to callShopifyAPI.Clients.withSession(<params>)
from an end-user experience perspective.