Skip to content

Add a showContents() method to Contents API #36

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

Merged
merged 4 commits into from
Mar 17, 2013

Conversation

winzou
Copy link
Contributor

@winzou winzou commented Mar 10, 2013

This is not directly linked to Github API.

This is a very useful method to get the real contents of a file, without having to do a bunch of tests by hand.

@stloyd
Copy link
Contributor

stloyd commented Mar 12, 2013

I'm not really sure about this TBH... Additionally I don't like the name, better would be one of those: download(), content(), body() or even get().

Also, can you add some tests for this? Thanks!

@winzou
Copy link
Contributor Author

winzou commented Mar 12, 2013

OK for download() instead of showContents(), I gave that name to be consistent with the show() method (that doesn't "show" anything either!). And I've added some test.

Honestly this is quite a usefull method, as you want to retrieve the content of a file more often than its information (default behavior of the API). And it's totally free ;)

$file = $this->show($username, $repository, $path, $reference);

if (!isset($file['type']) || 'file' !== $file['type']) {
throw new \Exception(sprintf('Path "%s" is not a file.', $path));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use library exceptions here & below, it makes easier when you use it in other projects. Also add @throws notes to phpdoc.

stloyd added a commit that referenced this pull request Mar 17, 2013
Add a showContents() method to Contents API
@stloyd stloyd merged commit 09b4523 into KnpLabs:master Mar 17, 2013
@stloyd
Copy link
Contributor

stloyd commented Mar 17, 2013

Merged, thanks!

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.

2 participants