search

saucelabs / sauce_whisk

ActiveRecord style client for the Sauce Labs RESTful API
https://opensource.saucelabs.com/sauce_whisk/
MIT License
20 stars 17 forks source link

Added delete_asset feature #31

Closed dmfranko closed 10 years ago

dmfranko commented 10 years ago

Hello!

This change supports deleting assets for a given job. Let me know if you can accept this or if there's a better way to implement this.

Thanks! Dan Franko

DylanLacey commented 10 years ago

Hey Dan,

Sorry for the delay in responding! This is a GREAT PR. I really appreciate it.

I've just got to double check that deleting files from the REST API is actually something that is meant to happen; I've heard some discussion recently about it at Sauce, and want to make sure we don't promise something we're not meant to deliver.

DylanLacey commented 10 years ago

All good!

dmfranko commented 10 years ago

Awesome! Happy I was able to contribute.

DylanLacey commented 10 years ago

Hey, just checking this out, what's the reason for returning an asset after deletion? Is your workflow:

  1. Run Job
  2. Fetch Job from API
  3. Delete Assets in API
  4. Store Assets locally

Or is there something else you're trying?

DylanLacey commented 10 years ago

Oh, and I renamed the method to delete, just because Assets.delete_asset was setting off my redundant duplication detector.

dmfranko commented 10 years ago

So the reason I named it delete_asset was because if I called it delete would get the below error, which I think is due to a name collision? Maybe I just didn't implement it correctly.

2.0.0-p195 :005 > SauceWhisk::Assets.delete "c7b9103163d8480595a7c50e5f58f62c" SystemStackError: stack level too deep from /Users/admin/.rvm/rubies/ruby-2.0.0-p195/lib/ruby/2.0.0/irb/workspace.rb:86 Maybe IRB bug!

If you called the same method before I made any of my changes it would delete the job since RestRequestBuilder was being extended in the Assets class.