Change used of NSString* to NSURL* in the public API

[marcomorain]

No description provided.

[marcomorain]

@evoL your fork seems to be the new home of AFAmazonS3Client :)

[evoL]

Well, I don’t consider myself a person responsible for the future of AFAmazonS3Client. :P
Because of that, I would prefer not to merge such breaking changes into my fork, or at least not into the version branches.

That being said, I think it’s worth to describe how I’d like to organize features around here:

• Version branches, such as 2.2, are supposed to maintain feature-parity with the official repo, but work properly with future AFNetworking versions.
• Additional functionality is kept on separate branches based on master, like session_token. This is for keeping potential upstream pull requests separate. Those versions do not work on 2.2.
• There are branches working with recent AFNetworking versions and with additional functionality merged in, as in 2.2-all_patches.

Anyway, care to elaborate more about your changes? What’s the motivation between changing NSString to NSURL? :)

[marcomorain]

I didn't really explain what I am dong here at all, did I? :)

I started trying to use the official AFAmazonS3Client along with the latest AFNetworking using cocoa pods and I ran into the incompatibility problems that your 2.2 branch fixes. I then ran into other problems getting PUT operations to work with a bucket that is not in the main S3 region (I have a bucket in EU-West-1). I'll annotate what those problems were in the diff shortly.

As for the change from NSString to NSURL, my code is dealing with uploading local files to S3. I reference the local files as NSURLs. When debugging I noticed that to call putObjectWithMethod I was having to convert my NSURL to an NSString, and then the very first line of code in setObjectWithMethod was converting the NSString back to a NSURL again. I was confused by this switching from URL to String, so I did a bit of reading to see what the most idiomatic thing to do is. http://nshipster.com/nsurl/ says to use NSURL. Apple say this: URL objects are the preferred way to refer to local files.

[marcomorain]

If the user passes a path does not exist locally here, then data will be null, and the PUT operation will not take place. This was causing my problems because setObjectWithMethod does not return anything to indicate failure, nor does it call the block passed as failure. The code was just failing silently. This was the first thing that I changed.

[evoL]

Wow. Thanks for a thorough explanation. :D

I have some minor issues with your changes though:

• Instead of simply replacing NSString with NSURL, you could introduce the NSURL versions alongside the NSString ones, making sure that existing code still works. I don’t mean duplicating code here, the NSString versions could very well call the NSURL ones with [NSURL fileURLWithPath:filePath], just as it is now.
• You’re fixing more than one problem here, so I’d suggest splitting this huge commit into smaller ones, allowing me to review and test each change one by one. That makes things easier. :)

I admit that I haven’t tested your code due to my busy schedule, but it would be great if you’d fix those things.

I’m going to find some time in the following days to play with your changes and maybe find some solutions for your sendSynchronousRequest: problem and the issue with XML error descriptions not being passed to the NSError object, so stay tuned :)

[marcomorain]

Awesome, thanks Rafal!

On Thu, May 8, 2014 at 2:55 PM, Rafał Hirsz notifications@github.comwrote:

Wow. Thanks for a thorough explanation. :D

I have some minor issues with your changes though:

• Instead of simply replacing NSString with NSURL, you could introduce
  the NSURL versions alongside the NSString ones, making sure that
  existing code still works. I don’t mean duplicating code here, the
  NSString versions could very well call the NSURL ones with [NSURL
  fileURLWithPath:filePath], just as it is now.
• You’re fixing more than one problem here, so I’d suggest splitting
  this huge commit into smaller ones, allowing me to review and test each
  change one by one. That makes things easier. :)

I admit that I haven’t tested your code due to my busy schedule, but it
would be great if you’d fix those things.

I’m going to find some time in the following days to play with your
changes and maybe find some solutions for your sendSynchronousRequest:problem and the issue with XML error descriptions not being passed to the
NSError object, so stay tuned :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-42552422
.