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

Add support for the list branched endpoint (47deg#231) #232

Merged
merged 7 commits into from
Dec 13, 2018

Conversation

YarekTyshchenko
Copy link
Contributor

@YarekTyshchenko YarekTyshchenko commented Dec 7, 2018

No description provided.

@JesusMtnez JesusMtnez added the enhancement New feature or request label Dec 10, 2018
@JesusMtnez
Copy link
Contributor

@YarekTyshchenko Apart from the minor typo, the decoders for the new entities (Branch and BranchCommit) are missing and tests are failing.

It should not be hard to add, but let me know if you need help doing so.

Copy link
Contributor

@JesusMtnez JesusMtnez left a comment

Choose a reason for hiding this comment

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

@YarekTyshchenko as it is described in the GitHub API Docs # list-branches, protected and protection* keys are only included in the JSON response if the query parameter protected is set to true in the request.

That makes the protected and protection_url in the response Option values, IMHO. I've just suggested those changes, but feel free to submit any other possible solution 👍

Also, I think it would be great to test both cases, with protected and without protected query parameter. What do you think?

@YarekTyshchenko
Copy link
Contributor Author

@JesusMtnez You are right, I must have missed that bit in the docs.Option sounds good!

Co-Authored-By: YarekTyshchenko <yarek@healthforge.io>
@YarekTyshchenko
Copy link
Contributor Author

I could use some help figuring out why the Travis tests are failing. I can't see anything obvious that I did wrong in Repos.scala. I'm not massively familiar with Scala yet

@fedefernandez
Copy link
Contributor

hey @YarekTyshchenko, the tests use a GitHub token that is not available in forks. You'd need to create a Personal Access token in Github with gists permissions (I don't remember if anyone more is necessary) and store it in the environment variable GITHUB4S_ACCESS_TOKEN before executing the tests. I can also execute them locally, I just need to find some time. Thanks

@JesusMtnez
Copy link
Contributor

@YarekTyshchenko Greenlight! All checks are passing, we are good to merge.

@JesusMtnez JesusMtnez merged commit 06bfced into 47degrees:master Dec 13, 2018
@JesusMtnez
Copy link
Contributor

@YarekTyshchenko do you need a new release of github4s? Just let me know if needed 😉

@YarekTyshchenko
Copy link
Contributor Author

@JesusMtnez Let me have a quick look at #233 first. I think that should be an easy one

@YarekTyshchenko YarekTyshchenko deleted the list-branches branch January 2, 2019 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants