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 methods for Domain Prices API #103

Merged
merged 10 commits into from
Apr 20, 2021

Conversation

duduribeiro
Copy link
Contributor

@duduribeiro duduribeiro commented Apr 10, 2021

This commit introduces a new client method for our Domain Prices API:

  • Registrar.GetDomainPrices - Retrieves the prices for a domain.

It also includes if the domain is premium or not in the response.

This commit introduces a new client method for our Domain Prices API:
- Registrar.GetDomainPrices - Retrieves the prices for a domain.

It also includes if the domain is premium or not in the response.
@duduribeiro duduribeiro added enhancement New feature, enhancement or code changes, not related to defects ready-for-review Pull requests that are ready to be reviewed by other team members. labels Apr 10, 2021
@duduribeiro duduribeiro self-assigned this Apr 10, 2021
Copy link
Contributor

@OleMchls OleMchls left a comment

Choose a reason for hiding this comment

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

👏 Just make sure CI passes before merging :)

CHANGELOG.md Outdated Show resolved Hide resolved
dnsimple/registrar.go Outdated Show resolved Hide resolved
@weppos
Copy link
Member

weppos commented Apr 13, 2021

Specs are broken. Please address them first, then re-add for review.

Copy link
Member

@weppos weppos left a comment

Choose a reason for hiding this comment

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

Specs are broken.

@weppos weppos requested a review from OleMchls April 13, 2021 10:40
@duduribeiro
Copy link
Contributor Author

Specs are broken.

The work to fix the cause of the broken specs are being done at #104.

Maybe we can allow 1.5 to fail for now until we finish the Transfer-Encoding work. wdyt @DXTimer @ggalmazor ?

@weppos
Copy link
Member

weppos commented Apr 13, 2021

Specs are broken.

The work to fix the cause of the broken specs are being done at #104.

Sorry, that was not clear (it was not mentioned in this PR). Perhaps you can temporarily add 1.15 to allow failures

dnsimple-go/.travis.yml

Lines 16 to 17 in 5d23d91

allow_failures:
- go: tip

dnsimple/registrar.go Outdated Show resolved Hide resolved
Co-authored-by: Santiago Traversa <san983@users.noreply.github.com>
Copy link
Contributor

@DXTimer DXTimer left a comment

Choose a reason for hiding this comment

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

Works like a charm

Data *DomainPrice `json:"data"`
}

func (s *RegistrarService) GetDomainPrices(ctx context.Context, accountID string, domainName string) (*DomainPriceResponse, error) {
Copy link
Member

Choose a reason for hiding this comment

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

You are missing the docs here. It breaks the lint.

➜  dnsimple-go git:(implement_domain_prices) ✗ golint ./...
dnsimple/registrar.go:110:1: exported method RegistrarService.GetDomainPrices should have comment or be unexported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weppos thanks!

shouldn't we add this on CI? thanks

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be great. Sadly, it's a bit complicated:

  1. I tried to add a Makefile in the past b7733b8, sadly once you add a Makefile Travis will rely entirely on it and you will need a non trivial amount of customization to make the build pass in both module and non-module versions. Things would be easier if we would only use module-aware versions (1.11 or above).
  2. golint is not enough to break the build. It's a highly recommended standard. We could add it as golint ./... in the travis.yml file, but it would cause the build to break if anything is invalid (and right now we have one lint warning). The current warning is actually a false positive due to a naming conflict between the webook package and the Webhook as result of the Webhook API. I am not sure how to ignore this as golint doesn't have a way to address false positives. We should probably upgrade to golangci-lint which instead does.

dnsimple/registrar.go Outdated Show resolved Hide resolved
Copy link
Member

@weppos weppos left a comment

Choose a reason for hiding this comment

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

👍

Please add the missing docs.

Co-authored-by: Simone Carletti <weppos@gmail.com>
@weppos weppos mentioned this pull request Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, enhancement or code changes, not related to defects 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.

5 participants