-
Notifications
You must be signed in to change notification settings - Fork 15
Adds support for both size and empty #9
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
Adds support for both size and empty #9
Conversation
Signed-off-by: Guyzmo <guyzmo+github+pub@m0g.net>
for Gogs/Gitea, API is not yet out, cf following PRs: cf unfoldingWord-dev/python-gitea-client#9 cf go-gitea/go-sdk#56 cf gogs/gogs#4484 cf gogs/go-gogs-client#64 cf https://github.com/gogits/gogs/pull/4484o fixes #129
for Gogs/Gitea, API is not yet out, cf following PRs: cf unfoldingWord-dev/python-gitea-client#9 cf go-gitea/go-sdk#56 cf gogs/gogs#4484 cf gogs/go-gogs-client#64 cf gogs/gogs#4484 fixes #129
for Gogs/Gitea, API is not yet out, cf following PRs: cf unfoldingWord-dev/python-gitea-client#9 cf go-gitea/go-sdk#56 cf go-gitea/gitea#1668 cf gogs/gogs#4484 cf gogs/go-gogs-client#64 fixes #129
|
@guyzmo Thanks! @ethantkoenig will do a code review soon, in the meantime can you augment the tests to maintain or increase the test code coverage? |
| def default_branch(self): | ||
| """ | ||
| The name of the default branch | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
gogs_client/entities.py
Outdated
| class GogsUser(object): | ||
| class GogsEntity(object): | ||
| def __init__(self, json): | ||
| self.json = json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- What is the point of this adding a parent class? I'm not necessarily opposed to it, but I don't see a clear benefit.
- Could we change this to
self._json, and add a@propertyfor it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because all API resources exposed shall store some JSON data, it seemed logical to offer a similar approach. Then improving like ② is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
gogs_client/entities.py
Outdated
| self._full_name = full_name | ||
| self._email = email | ||
| self._avatar_url = avatar_url | ||
| self._json = json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of this _json field? It's never used, and the JSON representation is already stored by the parent GogsEntity class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, that's totally useless 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
|
||
| @property | ||
| def size(self): | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you happen to know the unit of the size field (e.g. kilobytes, megabytes, etc.), please add it to the description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kb
Signed-off-by: Guyzmo <guyzmo+github+pub@m0g.net>
|
I made another change: instead of enforcing the |
ethantkoenig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cf go-gitea/go-sdk#56
cf gogs/gogs#4484
cf gogs/go-gogs-client#64
cf https://github.com/gogits/gogs/pull/4484o