Refactor Amazon Product API endpoints #174
Open
Conversation
This file contains 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
2 tasks
leesharma
force-pushed
the
api-client
branch
2 times, most recently
from
39f2301
to
908d53b
Compare
leesharma
changed the title
leesharma
force-pushed
the
api-client
branch
from
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
force-pushed
the
api-client
branch
from
54fa761
to
3b0bb6a
Compare
|
I'm all for modularization and templating, especially when it makes things easier in the future! 👍 |
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.)