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

added support for ref value in get contents api #730

Merged
merged 5 commits into from
Jul 24, 2015

Conversation

goalie7960
Copy link
Contributor

as far as i could tell, it was impossible to specify the ref parameter on the get contents api method https://developer.github.com/v3/repos/contents/#get-content

/// <returns>
/// A collection of <see cref="RepositoryContent"/> representing the content at the specified path
/// </returns>
public async Task<IReadOnlyList<RepositoryContent>> GetContents(string owner, string name, string path, string reference)
Copy link
Member

Choose a reason for hiding this comment

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

Because we maintain an set of IObservable<T> APIs that mirror this, the build server is currently complaining.

To fix this, you'll need to add the new method to IObservableRepositoryContentsClient and implement it over there too.

@shiftkey
Copy link
Member

shiftkey commented May 8, 2015

Would it be possible to add some tests for this? I'm not sure how valuable it'd be to target references that might be transient, but look at the integration tests in RepositoryContentsClientTests for some inspiration.

/// <returns>
/// A collection of <see cref="RepositoryContent"/> representing the content at the specified path
/// </returns>
IObservable<IReadOnlyList<RepositoryContent>> GetContents(string owner, string name, string path, string reference);
Copy link
Member

Choose a reason for hiding this comment

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

This should be IObservable<RepositoryContent>

@goalie7960
Copy link
Contributor Author

going to close this, will reopen when i'm ready

@goalie7960 goalie7960 closed this May 8, 2015
@goalie7960 goalie7960 reopened this May 8, 2015
/// <returns>
/// A collection of <see cref="RepositoryContent"/> representing the content at the specified path
/// </returns>
Task<IReadOnlyList<RepositoryContent>> GetContents(string owner, string name, string path, string reference);
Copy link
Member

Choose a reason for hiding this comment

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

In #771 we added a rule to the test suite to ensure all methods which returned a IReadOnlyList<T> were named with the prefix GetAll - if you update this name and the related observable method name you should be ✨

haacked added a commit that referenced this pull request Jul 24, 2015
added support for ref value in get contents api
@haacked haacked merged commit c64e397 into octokit:master Jul 24, 2015
@haacked
Copy link
Contributor

haacked commented Jul 24, 2015

selfie-0

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