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

Documentation and Example of GetReadme #323

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mbbroberg
Copy link
Contributor

mbbroberg commented Mar 30, 2016

I had a tough time figuring out how to view a readme through go-github. Adding a couple more notes on documentation for godoc to pick up and I added an example of doing so.

Fixes #322

mbbroberg added some commits Mar 30, 2016

Clarifying comments to address #322
While using `go-github`, I found using the RepositoryContent functions challenging to parse without a multiple detailed reads of the GitHub API. The primary challenge came from the results of GetReadme, which returns text in base64. To address it, I've added comments on this, and similar functions that return base64, suggesting that the user uses `Decode()`. `String()` was also commented since it does not result in a Readme in string format, which had me confused for a good hour or so.
Adding example of GetReadme command
This commit provides a simple how-to to parse an existing README.md file using the GetReadme command available through RepositoriesServices. It includes the different error handling I use, which have each come in handy.
@@ -72,7 +73,8 @@ func (r *RepositoryContent) Decode() ([]byte, error) {
return o, nil
}

// GetReadme gets the Readme file for the repository.
// GetReadme gets the Readme file for the repository, which is stored as base64.

This comment has been minimized.

@willnorris

willnorris Mar 31, 2016

Member

Rather than all these various doc changes, would it not be sufficient to just document the Content field of RepositoryContent to say that it may be encoded, and if so, then use Decode to decode it?

I don't really want to document that Readme is always stored as base64 because I don't actually know that that's true. Otherwise, why would GitHub bother returning an "encoding" value?

This comment has been minimized.

@mbbroberg

mbbroberg Apr 1, 2016

Author Contributor

Great point - I'll address this.

This comment has been minimized.

@mbbroberg

mbbroberg Apr 1, 2016

Author Contributor

I'm thinking what could be said safely is that it's retrieving encoded content. For instance: // GetReadme gets the encoded readme file for the repository.

Or was part of the point is you don't want to assume that it will remain encoded?

This comment has been minimized.

@mbbroberg

mbbroberg Apr 1, 2016

Author Contributor

As I type it up, I think it's great to have a note on RepositoryContent, though I want to give people a heads up as they look at each command. I could go either way, but I err on sharing wherever it hurt as a user.

This comment has been minimized.

@willnorris

willnorris Apr 2, 2016

Member

These methods clearly return a RepositoryContent struct, so wouldn't it be reasonable to expect a user to look at that type to then know what to do with it?

Nowhere else in go-github (that I can recall), does a method document how to use the returned type. Instead, the type itself documents what its fields are, special cases about when they may or may not be present, etc. I think the one exception may be where we mention that the caller is responsible for closing an io.ReadCloser similar to how the net/http documents that, but I may be misremembering that.

@@ -0,0 +1,32 @@
// Copyright 2016 The go-github AUTHORS. All rights reserved.

This comment has been minimized.

@willnorris

willnorris Mar 31, 2016

Member

could you change this to be an inline example, similar to what I did in 32a84d1?

This comment has been minimized.

@mbbroberg

mbbroberg Apr 1, 2016

Author Contributor

Glad to, though I don't know if I follow the definition of inline. Would it also go github/repos_test.go? Happy to have it land where you'd like it to live.

This comment has been minimized.

@willnorris

willnorris Apr 2, 2016

Member

close. actually in github/repos_contents_test.go, right after the existing TestRepositoriesService_GetReadme test. So we would just add a ExampleRepositoriesService_GetReadme there

This comment has been minimized.

@mbbroberg

mbbroberg Apr 3, 2016

Author Contributor

@willnorris working on this, but I'm not familiar with the convention of an example in test. From reading up here, it looks like I may need to change the package to github_test in order to import and run this line: client := github.NewClient(nil). If I do that, calls to setup, teardown and RepositoryContent are undefined.

What's the right move here? Outputs below:

### After changing the package to github_test ###
# github.com/google/go-github/github_test
./repos_contents_test.go:14: undefined: setup
./repos_contents_test.go:15: undefined: teardown
./repos_contents_test.go:16: undefined: RepositoryContent
./repos_contents_test.go:28: undefined: setup
./repos_contents_test.go:29: undefined: teardown
./repos_contents_test.go:30: undefined: RepositoryContent
./repos_contents_test.go:38: undefined: setup
./repos_contents_test.go:39: undefined: teardown
./repos_contents_test.go:40: undefined: mux in mux.HandleFunc
./repos_contents_test.go:41: undefined: testMethod
./repos_contents_test.go:41: too many errors
FAIL    github.com/google/go-github/github [build failed]

#### After importing and naming the package back to github ####
# github.com/google/go-github/github
import cycle not allowed in test
package github.com/google/go-github/github (test)
    imports github.com/google/go-github/github

FAIL    github.com/google/go-github/github [setup failed]

This comment has been minimized.

@mbbroberg

mbbroberg Apr 4, 2016

Author Contributor

Got it! I use setup & teardown now that I figured them out 👌

mbbroberg added some commits Apr 4, 2016

Better description and example of RepositoriesService's GetReadme
I added the language that highlights content returned as *mostlikely* (but not for sure) needing to be `.Decode()`-ed. This newly minted lesson-learned is captured as an example of collecting and reading the readme for this repository. May it prevent others from having similar I-dont-get-it moments like I did!
Removing separate example
This example was added to the repos_contents_test.go file as ExampleRepositoriesService_GetReadme(). RIP
@mbbroberg

This comment has been minimized.

Copy link
Contributor Author

mbbroberg commented Apr 4, 2016

I think we're on track now @willnorris. The edits we landed on are mostly in 02f6480. The only addition is a description for String() from 962a0f1. You're welcome to cherrypick (based on what I read in the repo) or lmk if you'd like some rebasing done on my end. I didn't want to lose your comments on past commits until you looked it over. Cheers 👍

willnorris added a commit to willnorris/go-github that referenced this pull request Apr 8, 2016

add RepoContent.GetContent to replace Decode
Decode only works if r.Content is actually encoded, which I'm not sure
that it always will be.  GetContent will return the content, decoding it
only if necessary.

Deprecate the old Decode method.

Related to google#323

willnorris added a commit to willnorris/go-github that referenced this pull request Apr 8, 2016

add RepoContent.GetContent to replace Decode
Decode only works if r.Content is actually encoded, which I'm not sure
that it always will be.  GetContent will return the content, decoding it
only if necessary.  Deprecate the old Decode method.

Also expand and cleanup the tests for both of these methods.

Related to google#323

willnorris added a commit to willnorris/go-github that referenced this pull request Apr 8, 2016

add RepoContent.GetContent to replace Decode
Decode only works if r.Content is actually encoded, which I'm not sure
that it always will be.  GetContent will return the content, decoding it
only if necessary.  Deprecate the old Decode method.

Also expand and cleanup the tests for both of these methods.

Related to google#323

willnorris added a commit that referenced this pull request Apr 8, 2016

add RepoContent.GetContent to replace Decode
Decode only works if r.Content is actually encoded, which I'm not sure
that it always will be.  GetContent will return the content, decoding it
only if necessary.  Deprecate the old Decode method.

Also expand and cleanup the tests for both of these methods.

Related to #323
@willnorris

This comment has been minimized.

Copy link
Member

willnorris commented Apr 8, 2016

With the new GetContents method in #331, this gets a lot simpler since callers should now always use that and not think at all about whether the content is encoded or not. Given that, I moved the docs down to the Content field and update the Example to use GetContents and simplified it a bit. Thanks for adding this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment