-
Notifications
You must be signed in to change notification settings - Fork 49
Comments
Refactor Amazon Product API endpoints#174
Open
leesharma wants to merge 3 commits intorubyforgood:master from
Open
Refactor Amazon Product API endpoints #174leesharma wants to merge 3 commits intorubyforgood:master from
leesharma wants to merge 3 commits intorubyforgood:master from
Conversation
@leesharma
leesharma
force-pushed
the
api-client
branch
from
October 26, 2017 18:27
013e514 to
2f91a89
Compare
2 tasks
@micahbales
micahbales
temporarily deployed
to
project-playtime-stagin-pr-174
November 21, 2017 03:33
Inactive
@leesharma
leesharma
force-pushed
the
api-client
branch
2 times, most recently
from
November 29, 2017 03:15
39f2301 to
908d53b
Compare
@leesharma
leesharma
changed the title
(削除) [Waiting on #173] Refactor Amazon Product API endpoints (削除ここまで)
(追記) Refactor Amazon Product API endpoints (追記ここまで)
Nov 29, 2017
@leesharma
leesharma
force-pushed
the
api-client
branch
from
November 29, 2017 03:40
908d53b to
54fa761
Compare
I'm pretty torn on this commit-an abstract superclass doesn't seem like a very ruby thing to do. Maybe I've been doing too much Java? There are two main goals of this commit: 1. It should be trivial to add new endpoints. We're going to have to add a few more, and being able to customize only the specialized bits will save a ton of time and prevent bugs. 2. Shared code should exist in one location. Already, we've got a bug in our signing process, and keeping this all in a superclass means we only need to fix it in one place. I don't like how I'm handling `@aws_credentials` at the moment (requiring each subclass to initialize it); any ideas of a better way to handle it?
Since the instructions are getting more complicated, this commit adds a README (with new endpoint instructions) to the Amazon Product API. This has three benefits: * It keeps the root README clean and relevant to most people. * It allows us to go into more documentation detail. * It facilitates an eventual extraction of the Amazon Product API lib. It might be worth extracting more from the "Getting Amazon Product Advertising API Working Locally" section into the lib README, but for now there's just endpoint information.
Whenever the tests run, the following error appeared: warning: already initialized constant AWSTestCredentials This moves the AWSTestCredentials init from the individual specs to a spec helper class: `spec/support/helpers/amazon_helpers.rb`.
@leesharma
leesharma
force-pushed
the
api-client
branch
from
November 29, 2017 03:47
54fa761 to
3b0bb6a
Compare
Collaborator
mxygem
commented
Nov 30, 2017
I'm all for modularization and templating, especially when it makes things easier in the future! 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works towards #62 (part 2/3)
Description
I broke #62 into three PRs for easier review: this is part 2. (part 1: #173)
There were two main changes in this PR: refactoring the endpoints and adding documentation.
Refactoring the Endpoints
I had two main goals here:
To do this, I extracted the general Amazon Product API logic into an
Endpointclass. Adding a new endpoint is now as simple as subclassingEndpointand specifying the request parameters and any response post-processing.Documentation
The
AmazonProductAPIlib is getting more complicated and self-contained, so I added aREADMEwith instructions on how to add a new endpoint. That way, we get the documentation without cluttering up the root README.At some point, it might be worth extracting more API-related instructions from the root README into here, but I held off for now.
Risks/Tradeoffs
I'm not sure how you all feel about abstract superclasses with subclass callbacks in ruby. 😬 I may have written too much Java lately.
I think this implementation makes the API wrapper easier to extend/maintain (especially since we know we'll need to add more endpoints/fix some signing bugs soon), but let me know if you think it's needlessly complicated. We can just skip this PR; it won't affect the issue.
Type of change
How Has This Been Tested?
No new behavior was added, but the specs in
spec/lib/amazon_product_api/*_spec.rbstill pass (+ the rest of the test suite.)