Skip to content

Conversation

necaris
Copy link

@necaris necaris commented Oct 20, 2017

Includes code stolen from @dsludwig (see dsludwig@68edeb4)

Closes #1978

Note this is still very incomplete and needs design discussion, which (as I understand it) should be happening on the issue?

@lunny lunny added modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Oct 21, 2017
@lunny lunny added this to the 1.x.x milestone Oct 21, 2017
Copy link
Member

Choose a reason for hiding this comment

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

This file should be put in github.com/go-gitea/git not here.

Copy link
Author

Choose a reason for hiding this comment

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

@lunny these types are for the API representation of the tree listing, matching the GitHub API, and to make the tree listing API easier to test -- otherwise they add little to the git.Tree and git.TreeEntry types that already exist. Would it be better to avoid defining public types, and instead add JSON annotations to those types in go-gitea/git ?

Copy link
Member

Choose a reason for hiding this comment

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

If we're sending them over to the Client they should be defined in go-gitea/go-sdk 🙂

Copy link
Member

Choose a reason for hiding this comment

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

How does GitHub API represent them? usually we create a APIFormat function for things like this

Copy link
Member

Choose a reason for hiding this comment

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

It's not though, since git cat-file prints the size along with the ref and filename :/

@HoffmannP
Copy link
Contributor

Is anyone currently working on this?

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 18, 2018
@lunny
Copy link
Member

lunny commented Nov 19, 2018

@HoffmannP I think no since this PR doesn't update for 1 month. @necaris are you still working on this?

@necaris
Copy link
Author

necaris commented Nov 19, 2018 via email

@lunny
Copy link
Member

lunny commented Nov 25, 2018

@necaris anothr PR #4185 also try to fix the issue. If you have too much time, maybe you could give some review on that and close this one?

@necaris
Copy link
Author

necaris commented Nov 25, 2018

@HoffmannP FYI, as @lunny suggested I'm going to close this in favor of #4185 which is already more complete -- looking at that code it looks like it's doing pretty much the same, but reinventing fewer data structures that have been added in the meantime. In case anyone finds this code useful I have rebased it to be up to date, though.

@necaris necaris closed this Nov 25, 2018
@lunny lunny removed this from the 1.x.x milestone Nov 25, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: file listing / tree access
4 participants