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

fix: warn host has been deprecated in favor of domain #76

Merged
merged 3 commits into from
Jul 21, 2020

Conversation

ericdeansanchez
Copy link
Contributor

The purpose of this PR is to warn users that host has been deprecated
in favor of domain.

lib/imgix/client.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

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

Sorry, one last thing. Can we also update the README calling out the deprecation and how to resolve it?

Everything else looks great 👍

@ericdeansanchez
Copy link
Contributor Author

ericdeansanchez commented Jul 17, 2020

@sherwinski Yeah, I was going to ask about that. Do you feel this is all coherent? I.e. domain getting reassigned to host, a warning is emitted each time host is used, but only through initialize.

This brings me to if domain is to be used, and [all?] references in the README are to be changed from host to domain, but really @host is the instance variable then anyone accessing client.domain might be getting something unexpected back, no?

Updating the readme and the above concern are separate matters for the most part; I had thought of the former but got sidetracked by the latter.

@sherwinski
Copy link
Contributor

@sherwinski Yeah, I was going to ask about that. Do you feel this is all coherent? I.e. domain getting reassigned to host, a warning is emitted each time host is used, but only through initialize.

This brings me to if domain is to be used, and [all?] references in the README are to be changed from host to domain, but really @host is the instance variable then anyone accessing client.domain might be getting something unexpected back, no?

Updating the readme and the above concern are separate matters for the most part; I had thought of the former but got sidetracked by the latter.

Just to close the loop on this: we settled on assigning an instance field @domain during init, in the case that users do try to access it.

@ericdeansanchez
Copy link
Contributor Author

@sherwinski I think I jumped the gun on the @host = @domain stuff. Totally missed that there were no accessors defined for @host to begin with 🙃 😬 🙃 TL;DR: we're good ha

@ericdeansanchez ericdeansanchez merged commit cd9ddf1 into main Jul 21, 2020
@ericdeansanchez ericdeansanchez deleted the deprecate-host branch July 21, 2020 18:36
ericdeansanchez pushed a commit to imgix/imgix-rails that referenced this pull request Jul 30, 2020
…rnings (#96)

The purpose of this PR is to resolve deprecation warnings being thrown
as a result of using imgix-rb version 3.3.0, see [here](imgix/imgix-rb#76).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants