Skip to content

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

Merged
merged 2 commits into from
May 4, 2017

Conversation

guyzmo
Copy link
Contributor

@guyzmo guyzmo commented May 4, 2017

@coveralls
Copy link

coveralls commented May 4, 2017

Coverage Status

Coverage decreased (-0.3%) to 93.728% when pulling bfd36b1 on guyzmo:features/new_api into 6fd4bf2 on unfoldingWord-dev:master.

guyzmo added a commit to guyzmo/git-repo that referenced this pull request May 4, 2017
guyzmo added a commit to guyzmo/git-repo that referenced this pull request May 4, 2017
guyzmo added a commit to guyzmo/git-repo that referenced this pull request May 4, 2017
@jag3773
Copy link
Contributor

jag3773 commented May 4, 2017

@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?

"""
The name of the default branch

:rtype: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

class GogsUser(object):
class GogsEntity(object):
def __init__(self, json):
self.json = json
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. 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.
  2. Could we change this to self._json, and add a @property for it?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

self._id = user_id
self._username = username
self._full_name = full_name
self._email = email
self._avatar_url = avatar_url
self._json = json
Copy link
Contributor

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.

Copy link
Contributor Author

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 😁

Copy link
Contributor Author

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):
"""
Size of the repository
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's kb

@guyzmo guyzmo force-pushed the features/new_api branch from bfd36b1 to bf5a6d8 Compare May 4, 2017 21:32
@guyzmo
Copy link
Contributor Author

guyzmo commented May 4, 2017

I made another change: instead of enforcing the size andempty attributes that are not yet released in neither gogs or gitea, I made them optional, at json parse time.

@coveralls
Copy link

coveralls commented May 4, 2017

Coverage Status

Coverage increased (+0.08%) to 94.097% when pulling bf5a6d8 on guyzmo:features/new_api into 6fd4bf2 on unfoldingWord-dev:master.

Copy link
Contributor

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

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

LGTM

@ethantkoenig ethantkoenig merged commit e56c6bc into unfoldingWord-dev:master May 4, 2017
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.

4 participants