Skip to content
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

Implement registrar/checkDomain #76

Merged
merged 7 commits into from
Mar 1, 2016
Merged

Conversation

jacegu
Copy link
Contributor

@jacegu jacegu commented Mar 1, 2016

No description provided.

@jacegu jacegu added the feature label Mar 1, 2016
@jacegu jacegu self-assigned this Mar 1, 2016
@jacegu jacegu added this to the API v2 milestone Mar 1, 2016
@jacegu
Copy link
Contributor Author

jacegu commented Mar 1, 2016

A couple things that I wanted to mention:

  1. The check vs. availability naming was somewhat confusing to me.
  2. For some reason I feel like the request should use domain_name instead of domain (I had the feeling that domain should be a Dnsimple::Struct::Domain instance).
  3. I maintained attributes and options to keep interfaces consistent even if this is a get request and without knowing if there is a use case for it.

@jacegu jacegu added the ready-for-review Pull requests that are ready to be reviewed by other team members. label Mar 1, 2016

def check(account_id, domain_name, attributes = {}, options = {})
options = options.merge(attributes)
response = client.get(Client.versioned("/%s/registrar/domains/%s/availability" % [account_id, domain_name]), options)
Copy link
Member

Choose a reason for hiding this comment

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

The correct path should be "/%s/registrar/domains/%s/check". Did you get this path from somewhere? It may be an outdated docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. At least in the RAML file. I think I saw it somewhere else. I will add it here if I find it.

Copy link
Member

Choose a reason for hiding this comment

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

I've fixed the RAML file, thanks.

@weppos
Copy link
Member

weppos commented Mar 1, 2016

The check vs. availability naming was somewhat confusing to me.

I agree, it partially is. We originally used /availability because /domains/check was too generic, but now that we moved all under /registrar I'm fine to keep reference "check".

In fact, I suggest to rename Structs::Availability to Structs::DomainCheck to maintain consistency across different clients.

@weppos
Copy link
Member

weppos commented Mar 1, 2016

For some reason I feel like the request should use domain_name instead of domain (I had the feeling that domain should be a Dnsimple::Struct::Domain instance).

Fine for me, in fact register already uses domain_name. FYI, consider we never accept a Domain struct in any call.

I maintained attributes and options to keep interfaces consistent even if this is a get request and without knowing if there is a use case for it.

Attributes is not required. There are no parameters in the payload. attributes generally represent a POST body payload.

@jacegu jacegu removed the ready-for-review Pull requests that are ready to be reviewed by other team members. label Mar 1, 2016
@jacegu
Copy link
Contributor Author

jacegu commented Mar 1, 2016

Fine for me, in fact register already uses domain_name. FYI, consider we never accept a Domain struct in any call.

I think I didn't explain myself here. What I mean is that Struct::Availability has a domain instead of a domain_name. This comes from the API endpoint directly so the change should be done on the API side so the serialization returns the name of the domain under domain_name in the payload.

@jacegu jacegu added the ready-for-review Pull requests that are ready to be reviewed by other team members. label Mar 1, 2016
@weppos
Copy link
Member

weppos commented Mar 1, 2016

:shipit:

jacegu added a commit that referenced this pull request Mar 1, 2016
@jacegu jacegu merged commit 304c61c into master Mar 1, 2016
@jacegu jacegu deleted the feature/registrar-check-domain branch March 1, 2016 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review Pull requests that are ready to be reviewed by other team members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants