-
Notifications
You must be signed in to change notification settings - Fork 465
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
ApiVersion::NullVersion tweaks #615
Conversation
lib/shopify_api/session.rb
Outdated
@@ -127,11 +127,11 @@ def site | |||
end | |||
|
|||
def api_version=(version) | |||
@api_version = version.nil? ? nil : ApiVersion.find_version(version) | |||
@api_version = version.nil? ? ApiVersion::NullVersion : ApiVersion.find_version(version) |
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.
api_version=
should work with nil
, ApiVersion::NullVersion
, any ApiVersion
and any String that is in an api version format.
Right now passing in ApiVersion::NullVersion
will raise. Making the below code impossible.
session_1= Session.new(domain: domain, url: url, api_version: nil)
session_2 = Session.new(domain: session_1.domain, url: session_1.url, api_version: session_1.api_version)
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.
guess this is a classic case "if its not broken, don't fix it". Session having a nil version has no negative affects as far as I can tell. Base having a nil version caused us issues, hence the NullVersion. I'll just remove this first commit.
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.
I think the coercion to out of primitives to ApiVersions is the right move.
This type of change should cover us.
@api_version = ApiVersion::NullVersion.matches?(version) ? ApiVersion::NullVersion : ApiVersion.find_version(version)
class NullVersion
def matches?(version)
version.nil? || version == self
end
end
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.
That sounds like a clean solution to me. Thanks! I just pushed it.
fb379da
to
f25afd0
Compare
f25afd0
to
7270b6d
Compare
1. Not strictly a bug, but Session defaults api_version to nil whereas Base defaults it to ApiVersion::NullVersion. For consistency sake, lets keep them the same.This PR prevents NullVersion from being instantiated and changes
find_version
to raise if passed the NullVersion class.