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

Failing tests for GetAllContentsByRef on a branch with a root path #1956

Closed
wants to merge 1 commit into from
Closed

Failing tests for GetAllContentsByRef on a branch with a root path #1956

wants to merge 1 commit into from

Conversation

SimonCropp
Copy link
Contributor

the "/" seems to force it back to the master branch. just wondering if this is by design?

var contents = await github
.Repository
.Content
.GetAllContentsByRef("octocat", "Spoon-Knife", "/", reference: "test-branch");
Copy link
Member

Choose a reason for hiding this comment

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

The URL that gets generated in here looks like this:

public static Uri RepositoryContent(string owner, string name, string path, string reference)
{
return "repos/{0}/{1}/contents/{2}?ref={3}".FormatUri(owner, name, path, reference);
}

I wonder if the "/" here is causing the confusing URL. @SimonCropp what happens if you make this an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null and empty string throw an arg exception. https://github.com/octokit/octokit.net/blob/master/Octokit/Clients/RepositoryContentsClient.cs#L103

happy to attempt a PR where either null or empty string mean "root dir"

@ryangribble
Copy link
Contributor

ryangribble commented Mar 30, 2019

I'm on mobile at the moment but I think there is another overload that doesn't take a path, which you can use when you want the root

@SimonCropp
Copy link
Contributor Author

@ryangribble yep and i am already using that workaround. although it makes for some ugly code for anyone hitting this

    static Task<IReadOnlyList<RepositoryContent>> GetAllContents(IRepositoryContentsClient content, string owner, string repo, string path, string branch)
    {
        if (branch == null)
        {
            if (path != null)
            {
                return content.GetAllContents(owner, repo, path);
            }
            return content.GetAllContents(owner, repo);
        }

        if (path != null)
        {
            return content.GetAllContentsByRef(owner, repo, path, reference: branch);
        }
        return content.GetAllContentsByRef(owner, repo, reference: branch);
    }

but ignoring that workaround. i think it is still a bug that you query for a branch and get results from master

@shiftkey
Copy link
Member

Moving this to #2105 as the fork has been removed

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