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

Deprecate Domain expires_on in favor of expires_at #135

Merged
merged 5 commits into from
Jun 19, 2020

Conversation

duduribeiro
Copy link
Contributor

@duduribeiro duduribeiro commented Jun 5, 2020

We deprecated expires_on attribute in our documentation and api in
favor of expires_at. This commit introduces the new field and removes
the old one. It also updates to use the new fixtures.

We should do a major bump, since we are removing an attribute.

Also synced these new fixtures with the docs one

We deprecated expires_on attribute in our documentation and api in
favor of expires_at. This commit introcuces the new field and removes
the old one. It also updates to use the new fixtures.
@duduribeiro duduribeiro added enhancement ready-for-review Pull requests that are ready to be reviewed by other team members. labels Jun 5, 2020
@duduribeiro duduribeiro self-assigned this Jun 5, 2020
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.

This would result in a breaking change and thus a new major release correct? Just want us to take that decision deliberately.

CHANGELOG.md Outdated Show resolved Hide resolved
@jacegu
Copy link
Contributor

jacegu commented Jun 8, 2020

@duduribeiro the only thing to keep in mind before releasing this is that clients should remain as much in sync as possible. So, if we end up not removing expires_on in the Ruby client, we should do the same in this one.

@duduribeiro
Copy link
Contributor Author

@jacegu and @OleMchls since I made change, wanted a new 👁️ on it.

@OleMchls with this new change, we will follow the same as we are doing in the other libraries, to not make a hard move and don't have a major bump.

@weppos
Copy link
Member

weppos commented Jun 9, 2020

@duduribeiro the behavior in all the clients must be consistent. So this PR (and all the other clients) needs to be updated to behave as dnsimple/dnsimple-ruby#186

@duduribeiro
Copy link
Contributor Author

@weppos @jacegu @OleMchls
Recently we dropped the domain expires_on on dnsimple-go by suggestion of @weppos, because dnsimple-go is not yet in 1.x, it wouldn’t be a problem to make another major release.
now on dnsimple-elixir, it is already at 2.0.0, we made a recent major release, so I made a little bit different from the dnsimple-go #135
I kept the expires_on, added a comment telling it is deprecated and added the expires_at. do you agree with it?

@duduribeiro duduribeiro merged commit 705d5e3 into master Jun 19, 2020
@duduribeiro duduribeiro deleted the include_expires_at branch June 19, 2020 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 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.

4 participants