-
Notifications
You must be signed in to change notification settings - Fork 22
Updated SDK #20
base: master
Are you sure you want to change the base?
Updated SDK #20
Conversation
@@ -7,14 +7,15 @@ | |||
it 'can describe an index that returns ids for the class type' do | |||
test_index = clazz.cloudsearch_index | |||
test_index.should be_kind_of(Cloudsearchable::Domain) | |||
test_index.fields.should have(4).items #3 explicit + 1 for the id of the object | |||
# test_index.fields.should have(4).items #3 explicit + 1 for the id of the object |
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.
Can you move the comment to the new line, and remove the commented-out line of code?
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.
Missed this comment. Sorry, I have removed the comment. I didn't mean to commit with the comment. I only had it during testing the change as reference.
Yehuda Katz suggests that libraries should not have Gemfile.lock in revision control. His argument makes sense to me. Can you either remove it, or explain why we want it? |
I had not seen that, but I just read over it and it makes sense. I have removed the Gemfile.lock file. |
Would you squash these three commits into one? |
Another contributor merged this functionality. So I'm closing this PR. Thanks for your contribution. |
Oh, I'm sorry. I completely forgot to squash the commits like you asked. My apologies. Thank you! |
Just wondering about this PR - it was getting somewhere to support the new API version (2013), but the other PR did not do this. Are there any plans to update to the new API version? |
I see what you're saying. In addition to updating the SDK gem to v2, you're also using the newer API version. Someone else's PR also updated the SDK gem to v2, so I prematurely closed your PR thinking that it duplicates the functionality. Sorry. Please let's finish the work of using the newer API! |
Also updated the specs since they were issuing deprecation warnings.