With this code it should be possible to tick the ‘Content’ box in issue
#37. The code is written after reviewing PR #64 and so is (very)
loosely based on that PR. So big thanks for the work already done by
@octavore! I did however had some different insights about a few fixes
and as the PR hasn’t moved for the last 5 months, I thought to just
rewrite and PR and I send in a new PR hoping to come to a merge…
Using an *Issue in IssuesService.{Create,Edit} means that the Labels
and Assignee fields do not serialize to the correct JSON representation
for the GitHub API. This change introduces an IssueRequest struct to
use instead, allowing labels and assignees to be set on create or edit.
Inspired by the Ruby version [1], UploadReleaseAsset now accepts a
simple path to a file.
The Content-Type is now determined internally using the
mime.TypeByExtension() function.
Note this version is also more flexible. Indeed, the previous version
needed an io.Reader, but actually only 3 kinds of io.Reader were bore.
This is because of http.NewRequest needs to determine the Content-Length
of the body, and it can only determine it for bytes.Buffer, bytes.Reader
or strings.Reader [2]. Note there is an issue raised about this [3].
With a path to a file, we can easily determine the file size, and then
manually add it to the request.
[1]
https://github.com/octokit/octokit.rb/blob/master/lib/octokit/client/releases.rb#L85
[2] http://golang.org/src/pkg/net/http/request.go#L447
[3] https://code.google.com/p/go/issues/detail?id=6738
this is a breaking change for a handful of Option structs that were
defining the Page option directly (they now embed ListOptions).
finishes fixing #56
ktoso kind of shamed me with his extensive resource tests in #49 :), so
I'm finally starting to setup a structure for more general testing of
this type. This change adds the `testJSONMarshal` helper function, and
adds tests for the `User` type. This includes two checks:
- check that an empty resource produces an empty JSON object. This
effectively verifies that all fields include 'omitempty' so that
we're not unintentionally sending additional data over the wire.
- check that a full resource with every field populated produces the
expected JSON. This verifies that the JSON field mappings are
correct. In this case, it might be okay to use resource samples from
the GitHub docs, though I do still prefer very simple field values
since it makes tests easier to read.
When these tests are added for each resource type, we can reduce all of
our other tests to return bare minimal response bodies, since the
resource fields are already being tested at that point.
- minor updates to README
- expand package docs to include more information on authentication, as
well as new sections for rate limiting and setting pointer values.
These tests exercise the code path in all of our API methods where an
error is returned from github.NewRequest(). There are actually only two
ways to trigger an error here:
- provide a URL which can't be parsed
- provide a request body which can't be JSON encoded
In theory, it's also possible that http.NewRequest() could return an
error which would make its way up the stack, but in practice this will
never happen. That's because http.NewRequest(), as currently
implemented, will only ever return an error if it's unable to parse the
provided URL. But since we're simply passing the output of
URL.String(), we know that it will never be unparseable. That leaves
the two options above.
For methods that don't have a request body (which is to say, most), all
we can do is force a URL parse error, which is most easily accomplished
by putting an unescaped "%" in the URL path. This works for methods
that take a string input parameter that is directly added to the path.
If there are no string parameters to the function, or if they're not
part of the URL path, then we're mostly out of luck. In some of those
cases, we can still try to force a JSON encoding error, but otherwise
there will be no way to exercise this code path in those methods.
Of course in this case, the fact that we can't test the error case means
that the error case isn't actually possible. Nevertheless, I don't want
to ignore the error entirely in case some future change in the
implementation of github.NewRequest() or http.NewRequest() might
introduce a new possibility for erroring out.
What we're testing here is not likely to be a common error case, and
adding all of these tests to check for it in each method may be a bit
OCD, but I'd rather have the test coverage.
UpdateComment now takes in a RepositoryComment instead of a string for a
parameter. This makes it easier to support more data types if/when the API
changes.
This provides somewhat reasonable string representations of GitHub
structs. This is specifically designed for this library and takes a
number of shortcuts where it can, so is not suitable as a general
purpose solution. That said, I am exporting this function because it is
useful for things like printing out slices of GitHub structs, as can be
seen in examples/example.go.
I am certainly open to suggestions for what exactly the stringified
output should look like. Currently, I think I've found a reasonable
compromise between fmt's "%v" and "%s" output.
Like a51d6b4303, this change makes me sad, mainly because it is a
breaking change for all clients, and makes common tasks like reading
data out of structs slightly more work, with no direct benefit. Notably,
developers will need to make sure and check for nil values before trying
to dereference these pointers. Sadly, the change is still necessary, as
is more fully explained in issue #19. We can make the nil pointer
checks a little easier by adding some Get* funcs like goprotobuf does.
I spent a lot of time over the last few weeks exploring this change
(switching fields to pointers) versus the much larger change of using
protocol buffers for all GitHub data types. While the goprotobuf
library is very mature and feature-rich (it's used heavily inside of
Google), it's the wrong tool for this task, since we're not actually
using the proto wire format. While it does address the immediate
concern in #19, it makes way too many other things terribly awkward.
One of the biggest drawbacks of this change is that it will make the
string output from fmt.Printf("%v") next to useless, since all pointer
values are displayed as their memory address. To handle that, I'll be
writing a custom String() function for these structs that is heavily
inspired by goprotobuf and internals from go's fmt package.
Sander van Harmelen
Tobias Schmidt
Chris Schaefer
Ondrej Kupka
Will Norris
Ali
Alex Bramley
Thomas Bruyelle
Zach Latta
Amey Sakhadeo
Shawn Smith
Quinn Slack
Alec Thomas
Will Norris
Konrad Malawski
Justin Abrahms
Billy Lynch
Daniel Leavitt
Arıl Bozoluk
Sean Wang