-
Notifications
You must be signed in to change notification settings - Fork 19
change from present?
(Rails) to native Ruby check
#57
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ | |
description: nil, | ||
properties: nil, | ||
multi_tenant: nil, | ||
auto_tenant_creation: nil, | ||
auto_tenant_activation: nil, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like it would be simpler to just default to def create(
...
multi_tenant: false,
auto_tenant_creation: false,
auto_tenant_activation: false And we'd just always set those values: req.body["multiTenancyConfig"] = {enabled: multi_tenant, autoTenantCreation: auto_tenant_creation, autoTenantActivation: auto_tenant_activation} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that it was strange that multi_tenant was the only arg that didn't match the weaviate arg, but didn't want to make a whole lot of changes in this PR. MultiTenancyConfig = Struct.new(:enabled, :autoTenantCreation, :autoTenantActivation) do
def initialize(enabled: false, autoTenantCreation: false, autoTenantActivation: false)
super
end
end create(
...
multi_tenancy_config: MultiTenancyConfig.new
) Since this can be used in For now, I'll go with your suggestion, because we are way off track of fixing the present? / blank? issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, just to be sure, you want me to blow away the existing code for this? I was trying to keep it backwards compatible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please! I think it's okay, I'll release a 0.10 to signify the breaking change here. |
||
vector_index_type: nil, | ||
vector_index_config: nil, | ||
vectorizer: nil, | ||
|
@@ -45,8 +47,10 @@ | |
req.body["properties"] = properties unless properties.nil? | ||
if multi_tenant.is_a?(Hash) | ||
req.body["multiTenancyConfig"] = multi_tenant | ||
elsif multi_tenant.present? | ||
req.body["multiTenancyConfig"] = {enabled: true} | ||
elsif !multi_tenant.nil? | ||
req.body["multiTenancyConfig"] = { enabled: true } | ||
Check failure on line 51 in lib/weaviate/schema.rb
|
||
req.body["multiTenancyConfig"].merge!(autoTenantCreation: true) unless auto_tenant_creation.nil? | ||
Check failure on line 52 in lib/weaviate/schema.rb
|
||
req.body["multiTenancyConfig"].merge!(autoTenantActivation: true) unless auto_tenant_activation.nil? | ||
Check failure on line 53 in lib/weaviate/schema.rb
|
||
end | ||
req.body["invertedIndexConfig"] = inverted_index_config unless inverted_index_config.nil? | ||
req.body["replicationConfig"] = replication_config unless replication_config.nil? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,31 @@ | |
) | ||
expect(response.dig("class")).to eq("Question") | ||
end | ||
|
||
context "when auto_tenant_creation passed" do | ||
before do | ||
@captured_request = nil | ||
allow_any_instance_of(Faraday::Connection).to receive(:post) do |_, path, &block| | ||
expect(path).to eq("schema") | ||
req = OpenStruct.new(body: {}) | ||
block.call(req) | ||
@captured_request = req | ||
response | ||
end | ||
end | ||
Comment on lines
+81
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. VCR or WebMock would simplify this a ton, but this is essentially grabbing the |
||
|
||
it "sets up multiTenancyConfig with autoTenantCreation and autoTenantActivation enabled" do | ||
schema.create( | ||
class_name: "Question", | ||
description: "Information from a Jeopardy! question", | ||
multi_tenant: true, | ||
auto_tenant_creation: true, | ||
auto_tenant_activation: true | ||
) | ||
|
||
expect(@captured_request.body["multiTenancyConfig"]).to eq({ enabled: true, autoTenantCreation: true, autoTenantActivation: true }) | ||
Check failure on line 101 in spec/weaviate/schema_spec.rb
|
||
end | ||
end | ||
end | ||
|
||
describe "#delete" do | ||
|
Uh oh!
There was an error while loading. Please reload this page.