Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Initial Version #2

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Initial Version #2

wants to merge 16 commits into from

Conversation

joaodaher
Copy link
Contributor

No description provided.

@joaodaher joaodaher added the enhancement New feature or request label Jun 28, 2019
@joaodaher joaodaher self-assigned this Jun 28, 2019
@joaodaher joaodaher requested a review from a team June 28, 2019 21:42
.gitignore Outdated Show resolved Hide resolved
.travis.yml Outdated
dist: xenial
sudo: true
python:
- "3.6"

Choose a reason for hiding this comment

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

Won't you support Python 2? 😱

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 RIP.

Choose a reason for hiding this comment

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

@property
def duration(self):
nanoseconds = self._get('Duration')
return timedelta(seconds=nanoseconds / 1000 / 1000 / 1000)

Choose a reason for hiding this comment

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

Suggestion: nanoseconds / 1e9

Choose a reason for hiding this comment

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

or nanoseconds / (1000 ** 3)

@joaodaher joaodaher requested a review from a team July 1, 2019 12:14
Copy link

@rodolfo3 rodolfo3 left a comment

Choose a reason for hiding this comment

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

I miss some separation between class methods and instance methods:

models.App.get(pk=666)
app = models.App()
app.get(pk=666)

It could clash some methods (django solves it using the .objects attribute).

I also see some repetition like:

    def user(self):
        return self._get('User')

But I'm not sure if it can be optimized.



class TestTsuruClient(HTTPPrettyTestMixin, unittest.TestCase):
@httpretty.activate

Choose a reason for hiding this comment

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

Isn't possible to use allow_net_connect=False?
https://httpretty.readthedocs.io/en/latest/api.html#enable


def setUp(self):
self.patcher = patch('tsuru.client.TsuruClient._URL', self.API_URL)
self.patcher.start()

Choose a reason for hiding this comment

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

You are not using self.patcher on tests, right?
So, you can avoid to save patcher on self using addCleanup:

patcher = patch('tsuru.client.TsuruClient._URL', self.API_URL)
patcher.start()
self.addCleanup(patcher.stop)

raise exceptions.UnexpectedDataFormat()

def refresh(self):
if not self._detailed:

Choose a reason for hiding this comment

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

what is this? why I cannot refresh the same model if _detailed is True?

Choose a reason for hiding this comment

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

I think we should rename this (refresh looks like refresh_from_db on a django model).

def test_list(self):
data = self.sample_list()
with self.patch_get(data=data) as get:
list(models.App.list())

Choose a reason for hiding this comment

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

I think this should test the returned value.

@property
def duration(self):
nanoseconds = self._get('Duration')
return timedelta(seconds=nanoseconds / 1000 / 1000 / 1000)

Choose a reason for hiding this comment

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

or nanoseconds / (1000 ** 3)


@property
def value(self):
return self._get('value')

Choose a reason for hiding this comment

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

Should we handle the **** if the value is not public?

@codeclimate
Copy link

codeclimate bot commented Apr 23, 2020

Code Climate has analyzed commit cbc0871 and detected 0 issues on this pull request.

View more on Code Climate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants