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
Enforce global name explicitly #20
Enforce global name explicitly #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -115,7 +116,6 @@ client = GarageClient::Client.new( | |||
adapter: :test, | |||
headers: { "Host" => "garage.example.com" }, | |||
endpoint: "http://localhost:3000", | |||
name: 'my-awesome-client', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 Good. I was confused by this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this 🙇
@@ -85,9 +85,6 @@ def require_necessaries(options) | |||
if !options[:endpoint] && !default_options.endpoint | |||
raise "Missing endpoint configuration" | |||
end | |||
if !options[:name] && !default_options.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure but why you droped checking default_options.name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because options[:name]
and default_options.name
isn't used in Client class.
Configuration#name
is used only in Configuration class itself, and the presence of name
is validated there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, then dropping these makes sense to me.
@cookpad/dev-infra @KOBA789 please have a look