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

[LIBCLOUD-747] Add Zonomi DNS provider implementation #582

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@aleGpereira
Contributor

aleGpereira commented Sep 18, 2015

No description provided.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 20, 2015

Member

Thanks for all those PRs and sorry for the delay (I've been traveling and was offline).

I will have a look as soon as the time permits.

Member

Kami commented Sep 20, 2015

Thanks for all those PRs and sorry for the delay (I've been traveling and was offline).

I will have a look as soon as the time permits.

@aleGpereira

This comment has been minimized.

Show comment
Hide comment
@aleGpereira

aleGpereira Sep 20, 2015

Contributor

It's fine. I have another one for monday or tuesday. That's the last one. Thank you guys for your awesome work!

Contributor

aleGpereira commented Sep 20, 2015

It's fine. I have another one for monday or tuesday. That's the last one. Thank you guys for your awesome work!

self.status = response.status
# This attribute is used when using LoggingConnection
original_data = getattr(response, '_original_data', None)

This comment has been minimized.

@Kami

Kami Sep 26, 2015

Member

I noticed you duplicated this code from the base class in one other driver as well. I know there is a small difference at the bottom, but duplication like this is problematic (it makes code hard to maintain). On top of the duplication, this also break the implicit signature / contract of the parse_body method (it should return an object, not a tuple).

I will review this and merge this today, but after this is merged can you please open a new PR which reduces the duplication - you should re-use as much functionality from the parent class as possible.

One thing you could do is make parse_body a no-op and add a new parse_body_and_errors method and make constructor look something like this:

def __init__(self, response, connection):
    super(Foo, self).__init__(response=response, connection=connection)

   self.objects, self.errors = self.parse_body_and_errors()
   if self.errors:
     raise self._make_excp(self.errors[0])
@Kami

Kami Sep 26, 2015

Member

I noticed you duplicated this code from the base class in one other driver as well. I know there is a small difference at the bottom, but duplication like this is problematic (it makes code hard to maintain). On top of the duplication, this also break the implicit signature / contract of the parse_body method (it should return an object, not a tuple).

I will review this and merge this today, but after this is merged can you please open a new PR which reduces the duplication - you should re-use as much functionality from the parent class as possible.

One thing you could do is make parse_body a no-op and add a new parse_body_and_errors method and make constructor look something like this:

def __init__(self, response, connection):
    super(Foo, self).__init__(response=response, connection=connection)

   self.objects, self.errors = self.parse_body_and_errors()
   if self.errors:
     raise self._make_excp(self.errors[0])

This comment has been minimized.

@aleGpereira

aleGpereira Sep 28, 2015

Contributor

Sure. Sorry for my bads! I'll look at this tomorrow. Thank you so much!

@aleGpereira

aleGpereira Sep 28, 2015

Contributor

Sure. Sorry for my bads! I'll look at this tomorrow. Thank you so much!

@asfgit asfgit closed this in f760e77 Sep 26, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 26, 2015

Member

Merged into trunk, thanks!

When time permits, please open a new PR (also for the other driver) which addresses this issue - https://github.com/apache/libcloud/pull/582/files#r40497873

Member

Kami commented Sep 26, 2015

Merged into trunk, thanks!

When time permits, please open a new PR (also for the other driver) which addresses this issue - https://github.com/apache/libcloud/pull/582/files#r40497873

@aleGpereira aleGpereira deleted the aleGpereira:LIBCLOUD-747_zonomi_provider branch Oct 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment