sdepold / octo.erl Goto Github PK

• List gists
• Get a single gist
• Create a gist
• Edit a gist
• List gist commits
• Star a gist
• Unstar a gist
• Check if a gist is starred
• Fork a gist
• List gist forks
• Delete a gist

• List your repositories
• List user repositories
• List organization repositories
• List all public repositories
• Create
• Get
• Edit
• List contributors
• List languages
• List Teams
• List Tags
• List Branches
• Get Branch
• Delete a Repository

In #22, @sdepold wrote:

Guess I will have to mock the test requests :(

So I've been looking into how others test their GitHub bindings, and it looks like the best way is employing two sets of tests:

• unit tests that only check that the URLs generated by the bindings are correct;
• integration tests that actually run against live GitHub.

Unit tests are run by Travis; integration ones are run by developers themselves. Integration tests require the developer to create an account for testing purposes.

What we have at the moment can be described as "integration tests implemented using unit testing framework". Furthermore, it's nailed to one specific repo, sdepold/octo.erl-test.

So here's an outline of what I'm (awfully lazily) doing:

1. (done) disentangle existing tests from sdepold's test repo. Encourage developers to create their own testing accounts;
2. migrate from EUnit to Common Test (questionable, but CT is for integration tests);
3. mock http_helper so that unit tests can be written in a proper way (this one will probably be influenced by my other slowly progressing work, namely turning http_helper into an instance of gen_server that will cache the responses).

Does that make sense?

• List issues
• List issues for a repository
• Get a single issue
• Create an issue
• Edit an issue
• Lock an issue
• Unlock an issue

Of all the functions that work with pull requests, merge_pull_request is the only one that doesn't lift JSON response into Erlang record. That should be fixed.

• Events
• Feeds
• Notifications
• Starring
• Watching

(This is a memo.)

So I looked into edoc, the goal being to see if it can:

• help us keep the docs closer to actual function definitions, and
• give us something more readable than a lo-ong list of available functions.

Edoc excels with the first goal, but totally fails the second.

I couldn't find a way to group functions—they're always in the alphabetical order.

I couldn't find a way to document two clauses differently—for each combination of function and its arity there's only one block of docs. This is really painful because we try to make pagination as unobtrusive as possible, which forces us to make things like octo:list_pull_requests(A, B) behave differently depending on the types of A and B. But if we can't document these behaviours separately, the docs are going to confuse people instead of enlightening them.

We'll have to keep using one big Markdown file for this, but I need to come up with a better, more readable formatting for it.

• Emojis
• Gitignore
• Markdown
• Meta
• Rate Limit
• Licenses

• Blobs
  • Get a Blob
  • Create a Blob
  • Custom media types
• Commits
  • Get a Commit
  • Create a Commit
• References
  • Get a Reference
  • Get all References
  • Create a Reference
  • Update a Reference
    • Make octo:update_reference take optional parameters again (removed in 957771a)
  • Delete a Reference
• Tags
  • Get a Tag
  • Create a Tag Object
• Trees
  • Get a Tree
  • Get a Tree Recursively
  • Create a Tree

This function turns refs/heads/A into A and refs/tags/B into B. It is sometimes applied to API responses.

I am starting to doubt its utility. I wholeheartedly agree that it's nice to write create_branch("user", "repo", "branchname") instead of create_reference("user", "repo", "refs/heads/branchname"), but I'd argue that responses should be the same regardless of what function we used. (And they are the same on API level, i.e. in JSON—it's Erlang side that's starting messing with things.)

#Some GitHub API endpoints accept optional arguments, e.g. body in "Create pull request" or all the stuff in "List pull requests". We don't have a set approach for handling those.

octo.erl itself has some internal options that are always passed in a list as last argument:

All_PRs = octo:list_pull_requests("sdepold", "octo.erl", [all_pages]).
%% Convenience overload passes empty list as options
Some_PRs = octo:list_pull_requests("sdepold", "octo.erl").

The question is: should I pass GitHub's options as a separate list (let's call this option №1), or should they be merged with octo.erl's options (options №2)?

Pros of option №1:

• no risk of name clashes between GitHub's and octo.erl's options.
• octo.erl can be dumb about handing GitHub's options: just serialize them.

Cons of option №1:

• a lot to type. Convenience overload that passes empty lists both as GitHub's and octo.erl's options is nice, but what if you need one but not the other? There's no named parameters in Erlang, so you'll have to write something like this:

  All_PRs = octo:list_pull_requests("sdepold", "octo.erl", [], [all_pages]).

• higher chance of breaking an API compatibility. Not all endpoints accept options, so not all of our functions will provide a parameter to pass them. But if GitHub adds some, it will be backward-compatible for them but not for us (because octo:f(X, Options) will suddenly become octo:f(X, Payload, Options).)

The options are symmetrical so the cons of option №1 are pros of option №2 and vice versa.

• Search repositories
• Search code
• Search issues
• Search users
• Text match metadata

• Get a single user
• Get the authenticated user
• Update the authenticated user
• Get all users

I'm told Rebar 3 is much, much better than Rebar 2. We should consider migrating. A migration tutorial is available.

• Enterprise Migrations
• Source Imports

The scope is not clear, need to go through the docs first.

This is inconsistent with the rest of the functions; an ordinary record should be returned, with message and documentation_url fields populated from the response.

GitHub provides access to certain yet-unreleased features if we pass some value in Accept header. Our users might need it, so we should provide a way to set the header safely.

GitHub docs make pretty poor job of describing what fields the returned objects will contain. The only way to get them all is to paste all example responses together and run them through bits of sed and uniq, which is quite lame in my opinion. And we do need to get them all, otherwise we'll hide parts of response from the user, which is bad.

Clearly, we have to abandon records altogether. Unfortunately, maps aren't fully ready yet, so we'll have to use dicts.

Any thoughts?

• List your organizations
• List user organizations
• List all organizations
• Get an organization
• Edit an organization
  • Make octo:update_organization take optional parameters again (removed in 3bb8dfc)

GitHub's API documentation specifically warns against the approach taken by octo.erl:

Keep in mind that you should always rely on these link relations provided to you. Don't try to guess or construct your own URL. Some API calls, like listing commits on a repository, use pagination results that are based on SHA values, not numbers.

Bumped into this while writing tests (yep, #24 ain't dead :), thought it's better to file an issue.

• List pull requests
• Get a single pull request
• Create a pull request
• Update a pull request
• List commits on a pull request
• List pull requests files
• Get if a pull request has been merged
• Merge a pull request (Merge Button)