Skip to content
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] Sequel integration #47

Merged
merged 41 commits into from
Dec 5, 2017
Merged

[WIP] Sequel integration #47

merged 41 commits into from
Dec 5, 2017

Conversation

nesaulov
Copy link
Owner

@nesaulov nesaulov commented Nov 16, 2017

#45 (with already merged #37)

# Conflicts:
#	Gemfile
#	gemfiles/activerecord42.gemfile
#	spec/collection_spec.rb
#	surrealist.gemspec
# Conflicts:
#	lib/surrealist/builder.rb
# Conflicts:
#	lib/surrealist/builder.rb
#	spec/collection_spec.rb
#	spec/orms/ar.rb
# Conflicts:
#	spec/orms/ar.rb
let(:collection) { SequelItem.all }

it { expect(Surrealist.surrealize_collection(collection))
.to eq([{ name: 'testing sequel' }].to_json) }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression at 18, 57 should be on its own line.

describe 'collections' do
let(:collection) { SequelItem.all }

it { expect(Surrealist.surrealize_collection(collection))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block body expression is on the same line as the block start.

Float :price
end


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line detected.

Repository owner deleted a comment from houndci-bot Nov 16, 2017
@nesaulov
Copy link
Owner Author

nesaulov commented Dec 4, 2017

@AlessandroMinali take a look. Actually i'm a bit exhausted by writing so much tests :)
Is there something that is not covered here from your point of view?
Glad to say that Sequel works perfectly by returning instances of models & plain Ruby arrays instead of some weird "association proxies". So no actual code was touched in this PR, only tests.

Copy link
Collaborator

@AlessandroMinali AlessandroMinali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I don't suspect much weirdness to pop out from sequel since it behaves quite predictably.

@nesaulov nesaulov merged commit 8b38823 into master Dec 5, 2017
@nesaulov nesaulov deleted the sequel_integration branch December 5, 2017 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants