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

Freeze constants and hash in services #70

Merged
merged 1 commit into from Jan 5, 2016

Conversation

leaexplores
Copy link
Contributor

This PR freezes constants located in the services implementations.

It should help a tad perf wise and avoid mutation errors.

Extracted from PR #68

[ 'Expedited Shipping', 'Expedited' ],
[ 'Priority Shipping', 'Priority' ]
].inject({}){|h, (k,v)| h[k] = v; h}
SHIPPING_METHODS
end
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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!

@leaexplores
Copy link
Contributor Author

Alright, I inlined freezed the 2 xml / json constants.

Let me know if there's more to inline.

Thanks!

garethson added a commit that referenced this pull request Jan 5, 2016
Freeze constants and hash in services
@garethson garethson merged commit 9747d4a into Shopify:master Jan 5, 2016
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.

None yet

3 participants