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
Freeze constants and hash in services #70
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
module ActiveFulfillment | ||
class ShopifyAPIService < Service | ||
|
||
OrderIdCutoffDate = Date.iso8601("2015-03-01") | ||
OrderIdCutoffDate = Date.iso8601('2015-03-01').freeze | ||
|
||
RESCUABLE_CONNECTION_ERRORS = [ | ||
Net::ReadTimeout, | ||
|
@@ -28,7 +28,7 @@ class ShopifyAPIService < Service | |
ActiveUtils::ConnectionError, | ||
ActiveUtils::ResponseError, | ||
ActiveUtils::InvalidResponseError | ||
] | ||
].freeze | ||
|
||
def initialize(options = {}) | ||
@name = options[:name] | ||
|
@@ -94,11 +94,11 @@ def send_app_request(action, headers, data) | |
|
||
def parse_response(response, root, type, key, value) | ||
case @format | ||
when 'json' | ||
when 'json'.freeze | ||
response_data = ActiveSupport::JSON.decode(response) | ||
return {} unless response_data.is_a?(Hash) | ||
response_data[root.underscore] || response_data | ||
when 'xml' | ||
when 'xml'.freeze | ||
response_data = {} | ||
document = REXML::Document.new(response) | ||
document.elements[root].each do |node| | ||
|
@@ -115,9 +115,9 @@ def parse_response(response, root, type, key, value) | |
|
||
def encode_payload(payload, root) | ||
case @format | ||
when 'json' | ||
when 'json'.freeze | ||
{root => payload}.to_json | ||
when 'xml' | ||
when 'xml'.freeze | ||
payload.to_xml(:root => root) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might be over doing the constants, @garethson ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, see my comments in #71. We should really only be creating constants if they're used in more than one place, otherwise just freezing strings inline is my preference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done for those 2. JSON_DATATYPE and XML_DATATYPE. |
||
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.
@garethson do we use this method? Thoughts on putting a note in the change log and making this a bigger version bump to just use the constant?
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.
@kevinhughes27 We do use this method, and a couple of the providers actually do run code to generate it. So maybe leave that change for later.
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.
okay, definitely leave the method then!