Skip to content

Added symlink functionality via X-Object-Manifest header management#565

Merged
ycombinator merged 6 commits intorackspace:workingfrom
markchalloner:working
Mar 23, 2015
Merged

Added symlink functionality via X-Object-Manifest header management#565
ycombinator merged 6 commits intorackspace:workingfrom
markchalloner:working

Conversation

@markchalloner
Copy link
Copy Markdown
Contributor

Implementation of #561

@ycombinator
Copy link
Copy Markdown
Contributor

Thanks for the PR, @markchalloner! We'll review it shortly and provide any feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a description for this parameter? We are starting to auto-generate API documentation so the description would be useful to readers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

…Manifest protected. Updated PHPDoc and tests.
@ycombinator
Copy link
Copy Markdown
Contributor

I suspect I need to add a mock response somewhere? I'm not entirely sure what the correct way of doing this is. Any pointers?

You are correct about setting up a mock response before your unit test calls createSymlinkFrom/To. You can see some examples of setting up mock responses in here: https://github.com/rackspace/php-opencloud/blob/working/tests/OpenCloud/Tests/ObjectStore/Resource/ContainerTest.php. Search the file for $this->addMockSubscriber(.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning null for an error case, lets throw an exception instead. This has the following benefits:

  • it will keep error-handling consistent within this method. Just as you throw Exceptions\NoNameError when the object has no name, you can throw another exception if the object has content in it.
  • by using an exception instead of returning null, we can provide more information to the caller about what went wrong.

Feel free to re-use an exception class from https://github.com/markchalloner/php-opencloud/tree/working/lib/OpenCloud/Common/Exceptions or add your own if you can't find one that's appropriate.

This same comment applies to the createSymlinkFrom method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @markchalloner, could you please remove the null from the @return type and description? Also, could you please add the necessary @throws documentation for the exceptions thrown by this method?

@ycombinator
Copy link
Copy Markdown
Contributor

Hey @markchalloner, this is looking quite good. I have made a couple of minor suggestions about using exceptions instead of returning nulls, and I've provided a pointer to setting up mock responses. Once those changes are in, I'll be happy to merge this PR. Thanks!

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.16%) to 88.15% when pulling ceebd95 on markchalloner:working into 8105fa1 on rackspace:working.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @markchalloner, could you please remove the null from the @return type and description? Also, could you please add the necessary @throws documentation for the exceptions thrown by this method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops missed that. Updated now. Cheers

@ycombinator
Copy link
Copy Markdown
Contributor

@markchalloner Thanks for the unit test using MockSubscriber. You are right about the number of requests - it is quite a lot! Unfortunately, this is a bit of a known issue with this version of php-opencloud. I can't think of a way to work around it so I would keep your code as-is for now. We are currently in the early stages of planning a newer, much more efficient PHP SDK. In this new SDK, we'll be sure to provide the object symlinking capability as efficiently as possible.

I've made two minor comments re: docblocks. Once those are addressed, I'll merge this PR. Very nice work on this with the implementation, tests and documentation!

ycombinator added a commit that referenced this pull request Mar 23, 2015
Added symlink functionality via X-Object-Manifest header management
@ycombinator ycombinator merged commit 412229a into rackspace:working Mar 23, 2015
@ycombinator
Copy link
Copy Markdown
Contributor

Once again, awesome job @markchalloner. Thank you for this contribution!

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.

3 participants