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

Move to Faraday #24

Closed
wants to merge 34 commits into from
Closed

Move to Faraday #24

wants to merge 34 commits into from

Conversation

trobrock
Copy link
Collaborator

Currently, the gem is doing a lot of the HTTP work on its own, which might not be such a good idea (e.g. issue #23). Moving to Faraday would also take a good chunk out of the test code, and permit easier future extension. Thoughts and opinions?

@reagent
Copy link
Collaborator

reagent commented Feb 20, 2012

I want to minimize the number of external dependencies this gem has, which is why I went the net/http route in the beginning. With your patch to #23, there shouldn't be much need for these other dependencies -- are there other issues that would be resolved by adding one of these gems?

@Muon
Copy link
Collaborator Author

Muon commented Feb 20, 2012

I should think that the lack of error detection and handling and write support could be much more easily remedied with Faraday. Also, parallel requests could be easily integrated with the Typhoeus adapter.

@Muon
Copy link
Collaborator Author

Muon commented Mar 1, 2012

One more point for Faraday: the JSON parsing middleware doesn't seem to have any Rails-related bugs.

@trobrock
Copy link
Collaborator

trobrock commented Jul 3, 2012

I'd be interested in seeing what the spike of this would look like, have you done any work towards this to see how involved it would be?

@kytrinyx
Copy link
Collaborator

kytrinyx commented Jul 4, 2012

+1, I'd also be interested in seeing a spike on this.

@Muon
Copy link
Collaborator Author

Muon commented Jul 4, 2012

Sorry, nothing yet. Should be pretty simple though.

@trobrock
Copy link
Collaborator

I'm spiking on a new version of the gem, this include a client class like mentioned in #29 and using Faraday. Its on the faraday branch if you are interested. https://github.com/kytrinyx/etsy/tree/faraday

@kytrinyx
Copy link
Collaborator

I really like this direction. I'll jump in tomorrow and add the pieces for authenticated requests.

@Muon
Copy link
Collaborator Author

Muon commented Jul 18, 2012

Seconded. I'll see if I can find a bit of time to help with this.

@kytrinyx
Copy link
Collaborator

I'm looking at the VCR / API key things for testing, and wondering how to best set up tests for an authenticated user. We can use ENV variables so that each developer keeps track of their own API key, but what about having a test user and test shop?

I could write specs against my own user, but I don't want to commit expected results to the repo. We could filter out the sensitive data in the same way that we filter out the API key, but then you couldn't delete the cassettes without knowing how to log in as me.

Should we create a fake user and share the login amongst us? (not a very tempting proposition).

Any suggestions?

@trobrock
Copy link
Collaborator

The way I've been doing it is filling in my own API key in the
api_credentials.rb file and VCR will auto filter out things defined as a
constant in those files, just don't commit the one file.
On Jul 18, 2012 6:03 AM, "Katrina Owen" <
[email protected]>
wrote:

I'm looking at the VCR / API key things for testing, and wondering how
to best set up tests for an authenticated user. We can use ENV variables
so that each developer keeps track of their own API key, but what about
having a test user and test shop?

I could write specs against my own user, but I don't want to commit
expected results to the repo. We could filter out the sensitive data in the
same way that we filter out the API key, but then you couldn't delete the
cassettes without knowing how to log in as me.

Should we create a fake user and share the login amongst us? (not a very
tempting proposition).

Any suggestions?


Reply to this email directly or view it on GitHub:
#24 (comment)

@trobrock
Copy link
Collaborator

If that doesn't make sense let me know

@kytrinyx
Copy link
Collaborator

It makes sense, but I'd rather not have to remember not to commit a change to a file that is already in source control. I'd rather export SOME_CONSTANT in .bash_profile and then ENV.fetch("SOME_CONSTANT") in the code.

@michaelficarra
Copy link

No need to export. SOME_CONSTANT=secret ruby myprogram.rb.

@trobrock
Copy link
Collaborator

I'll push a change in a few minutes for the env variables
On Jul 18, 2012 11:09 AM, "Michael Ficarra" <
[email protected]>
wrote:

No need to export. SOME_CONSTANT=secret ruby myprogram.rb.


Reply to this email directly or view it on GitHub:
#24 (comment)

@trobrock
Copy link
Collaborator

What do you think about this: 31d128b

@kytrinyx
Copy link
Collaborator

Oh, nice!

@kytrinyx
Copy link
Collaborator

@Muon do you see a need for a single Etsy::Client instance that could work with separate oauth credentials? If so, what would you want that to look like? A Credentials object that can be passed to the Client instance along with the call?

@Muon
Copy link
Collaborator Author

Muon commented Jul 21, 2012

No, no, this is fine. I was merely confused since there were attr_accessors defined for oauth_token and oauth_secret. So should they be removed, or at least replaced with attr_readers?

@trobrock
Copy link
Collaborator

Right now the way it works is a bit weird you have to use the access token
method to get the keys and then you have to set them on the client class
via the attr_accessors.
On Jul 21, 2012 6:08 AM, "Mak Nazečić-Andrlon" <
[email protected]>
wrote:

No, no, this is fine. I was merely confused since there were
attr_accessors defined for oauth_token and oauth_secret. So should
they be removed, or at least replaced with attr_readers?


Reply to this email directly or view it on GitHub:
#24 (comment)

@kytrinyx
Copy link
Collaborator

Basically there are a couple ways this would be used.

  1. Create a client, do the oauth dance, and continue using the client
  2. Initialize the client with the oauth token/secret

It might make sense to save the oauth token into instance variables when they're received and only have attr_readers, like @Muon suggested.

@trobrock
Copy link
Collaborator

I agree
On Jul 21, 2012 10:47 AM, "Katrina Owen" <
[email protected]>
wrote:

Basically there are a couple ways this would be used.

  1. Create a client, do the oauth dance, and continue using the client
  2. Initialize the client with the oauth token/secret

It might make sense to save the oauth token into instance variables when
they're received and only have attr_readers, like @Muon suggested.


Reply to this email directly or view it on GitHub:
#24 (comment)

@travisbot
Copy link

This pull request fails (merged f5f2370 into 04416c2).

@travisbot
Copy link

This pull request passes (merged 0c5074b into 04416c2).

@trobrock
Copy link
Collaborator

@kytrinyx what's left to do here, sorry been a bit disconnected lately
On Aug 21, 2012 12:05 PM, "The Travis Bot" [email protected] wrote:

This pull request passeshttp://travis-ci.org/kytrinyx/etsy/builds/2192393(merged
0c5074b 0c5074b0 into 04416c204416c2c
).


Reply to this email directly or view it on GitHubhttps://github.com//pull/24#issuecomment-7912263.

@kytrinyx
Copy link
Collaborator

@trobrock I think the only thing left is to add support for nested resources:

Finally, associations can be nested up to three levels deep, using slashes:

/v2/listings/active?includes=Shop/User
/v2/shops/etsystore?includes=Listings:1:0/Images

@travisbot
Copy link

This pull request passes (merged 3e20b95 into 04416c2).

@kytrinyx
Copy link
Collaborator

@trobrock I've added the nested resources syntax to the query.

If this all looks good, I'd suggest that we merge this in, deprecate the model stuff (but not rip it out yet), and bump the version to 0.3.0. Then when we're ready to remove the older syntax we can bump to 1.0.0.

Would you have time over the next week or so to take the query syntax for a spin?

@trobrock
Copy link
Collaborator

Looks good to me, I will try this week or next to test out the syntax. I agree with the idea of one release with a deprecation, then completely remove it.

kytrinyx and others added 6 commits September 23, 2012 14:47
* master:
  #53: add bought listings to user
  pass credentials
  #51: find admirers
  Inline the Etsy.host parameter in BasicClient
  add Roger Smith to README
  add more attributes to listing
  add more attributes to listing
  ability to create a favorite listing
  ability to create a favorite listing
  add buyer_id to transactions
  should be users/ not listings/
  adding ability to retrieve a user's favorite listings
  Updating rake dependency for upcoming 0.9.3 release
When running the specs on jruby, srand is initialized incorrectly.
Downgrading to rspec 2.7.x allows the specs to be run.
@trobrock trobrock closed this Jul 5, 2020
@trobrock trobrock deleted the faraday branch July 5, 2020 11:31
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.

7 participants