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 create tree method #71

Merged
merged 4 commits into from
Apr 30, 2017
Merged

Add create tree method #71

merged 4 commits into from
Apr 30, 2017

Conversation

dcaunt
Copy link
Collaborator

@dcaunt dcaunt commented Apr 26, 2017

This makes some changes to the internal send method to accept an Encodable but no public API changes.

I’m not sure about the name NewTree for the internal type encoded in the request – any ideas?

dcaunt added 2 commits April 26, 2017 19:28
This makes some changes to the `internal` `send` method to accept an
`Encodable` but no public API changes.

I’m not sure about the name `NewTree` for the internal type encoded in
the request – any ideas?
/// Create a tree in a repository
public func create(tree: [Tree.Entry], basedOn base: String?, in repository: Repository, inBranch branch: String? = nil) -> SignalProducer<(Response, FileResponse), Error> {
let newTree = NewTree(entries: tree, base: base)
return send(newTree, to: .tree(owner: repository.owner, repository: repository.name, recursive: false, ref: nil), using: .post)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is supposed to be ref: branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, the branch argument should not be there. The tree endpoint is used for both getting and creating a tree, where the path takes the following formats respectively:

  • /repos/:owner/:repo/git/trees/:sha
  • /repos/:owner/:repo/git/trees

when creating the tree, the base is part of the NewTree, rather than a ref for Endpoint.tree.

We could create a new Endpoint createTree if you think this would be clearer.


internal func encode() -> JSON {
var payload: [String: JSON] = [
"tree": .array(entries.map{ $0.encode() })
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a space between map and {?

@mdiep
Copy link
Owner

mdiep commented Apr 29, 2017

Looks good to me, apart from the error above. 👍

I have some changes I want to make to the API as a whole, but I'll wait for this PR before I try to make any changes.

@dcaunt
Copy link
Collaborator Author

dcaunt commented Apr 30, 2017

What do you have in mind for the API changes?

I'd like to add support for blobs next. These are interesting because they can be fetched as raw content, or as JSON describing the blob. The core fetch(_:) method needs to change in this case because a bad request still results in a standard JSON error response, but otherwise we want to return raw Data rather than deserialize an object. This may not be related to what you're working on or considering, but I thought it worth mentioning.

@mdiep mdiep merged commit 660f945 into mdiep:master Apr 30, 2017
@mdiep
Copy link
Owner

mdiep commented Apr 30, 2017

What do you have in mind for the API changes?

I want to move to a value-based approach like I mentioned in my talk at the functional swift conference. I'm currently working on what that API should be exactly—and how much, if any, should reside in a separate framework.

@dcaunt
Copy link
Collaborator Author

dcaunt commented Apr 30, 2017

I was hoping you'd say that 😀

@dcaunt dcaunt deleted the create-tree branch April 30, 2017 15:54
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.

2 participants