Force Gem Best Practice #229
Conversation
| @@ -21,11 +21,20 @@ def service(new_service, constant_string) | |||
| Fog.services[new_service] ||= [] | |||
| Fog.services[new_service] |= [to_s.split("::").last.downcase.to_sym] | |||
| @services_registry ||= {} | |||
| @services_registry[new_service] = [to_s, constant_string].join("::") | |||
| @services_registry[new_service] = service_klass(constant_string) | |||
There was a problem hiding this comment.
I'm not sure how this is working now. As of my understanding it will always load services in the form of Fog::<PROVIDER>::<Service> and we have many providers doing Fog::<Service>::<PROVIDER>
There was a problem hiding this comment.
If I recall correctly, it was all Fog::<PROVIDER>::<SERVICE> initially, and we might have changed some along the way in the hopes that the other way was better? It would be best, I suspect, to pick one and stick to it though (which would probably be the old way). What do you think?
There was a problem hiding this comment.
I agree to sticking to one. So my idea is to deprecate the one that might give us problems (2 different providers define a class named Fog::Compute::Utils) and drop support in a major version. This is being done from the service_klass method.
lib/fog/core/services_mixin.rb
Outdated
| def require_service_provider_library(service, provider) | ||
| require "fog/#{provider}/#{service}" | ||
| rescue LoadError # Try to require the service provider in an alternate location | ||
| Fog::Logger.deprecation('Prefer to define all Services within the Provider Namespace instead!') |
There was a problem hiding this comment.
I think maybe a more specific error would be helpful here. Like Unable to load "#{fog/#{provider}/#{service}". What do you think?
There was a problem hiding this comment.
Hmmm. Good idea. How to announce the deprecation as well?
lib/fog/core/services_mixin.rb
Outdated
| Fog.const_get(provider_name).const_get(*const_get_args(service_name)) | ||
| rescue NameError # Try to find the constant from in an alternate location | ||
| Fog::Logger.deprecation('Prefer to define all Services within the Provider Namespace instead!') |
There was a problem hiding this comment.
Similarly, a more specific error here would probably be less confusing, though I'm much less sure how to word it helpfully here. What do you think?
spec/spec_helper.rb
Outdated
| @@ -10,6 +10,7 @@ | |||
| require "minitest/autorun" | |||
| require "minitest/spec" | |||
| require "minitest/stub_const" | |||
| require "pry" | |||
There was a problem hiding this comment.
I suspect it may have been unintentional to commit this?
There was a problem hiding this comment.
Ops. Good catch. =)
|
Will work on your comments later today. Thanks for the review! |
All classes defined from within a Gem should be inside its own namespace to avoid conflicts with other gems
plribeiro3000
force-pushed
the
best-practices
branch
from
90c22c5
to
473abf8
Compare
|
Here we go @geemus. I ended up doing 2 messages, one to tell it could not load the class and another one to notify about the deprecation. What do you think about this approach? Maybe too verbose? |
|
Looks good, thanks! |
Following fog/fog-core#229, this gem needs to be namespaced.
All classes defined from within a Gem should be inside its own namespace
to avoid conflicts with other gems
cc/ @geemus @icco @Temikus @lanej @fog/core
This will be the first PR of many to