-
Notifications
You must be signed in to change notification settings - Fork 345
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
[WIP] 2.0.0 changes #336
[WIP] 2.0.0 changes #336
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.
Only some small things, the rest looks good to me.
|
||
## Installation | ||
|
||
Add the gem to your Gemfile | ||
|
||
``` ruby | ||
gem 'asset_sync' | ||
gem "asset_sync" |
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 think that can be removed? Because you always need to add a provider which is described below?
If you prefer to load fewer classes into your application, which will reduce load time and memory use, | ||
you need to load those parts of Fog yourself *before* loading AssetSync: | ||
Asset Sync has no idea about what provider will be used, | ||
so you are responsible for bundling the right gem for the provided to be used. |
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.
"... the provider to be used."
``` | ||
|
||
If you don't install the required gem, | ||
Fog will complain (by exception) about it when provider is set by Asset Sync. |
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 don't know if you should keep a require "fog/core"
in the lib/asset_sync.rb
. Normally it shouldn't be needed, because if you add a provider, the provider requires it. But If you don't do that, then Fog can't complain about it, because Asset Sync doesn't even find Fog.
@SuperTux88 |
@@ -1,4 +1,6 @@ | |||
require File.dirname(__FILE__) + '/../spec_helper' | |||
require "fog/core" |
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.
This isn't needed anymore.
@@ -1,4 +1,5 @@ | |||
require File.dirname(__FILE__) + '/../spec_helper' | |||
require "fog/core" |
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.
Some here.
@SuperTux88 I have given up on making integration spec green. |
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.
Doc and code looks good to me now.
I will release this within this week (unless I forgot o_0 |
About the spec: I haven't tested anything, only reviewed the changes. But I assume that it is also broken without your changes? |
The integration fails since complains about access key / secret key missing |
e8deafd
to
3f50a11
Compare
@SuperTux88 |
Main changes
fog-core
instead offog