Add support for Nobrainer (RethinkDB) #522

zedtux · 2018-01-12T10:42:35Z

Add persistence for NoBrainer
Add tests for NoBrainer
Show the ORM version when running the tests
Add a Dockerfile and a docker-compose.yml file allowing to run from anywhere very easily the test suite with `docker-compose run --rm aasm'
Moves the Mongoid database name definition in the gem loader (avoiding code repetition)
Update the CHANGELOG.md and the README.md files
Add Rails generator for NoBrainer

FYI I had to create new Appraisals for NoBrainer as there is a conflict with Mongoid.

This contribution is in the name of Pharmony!

zedtux · 2018-01-12T11:21:22Z

The builds for the Rubies are passing now! Ready for merge 😎

anilmaurya · 2018-01-12T11:27:04Z

@zedtux Thank you for your efforts 👍

I will review and merge in couple of days .

nviennot · 2018-01-12T15:12:43Z

spec/unit/persistence/no_brainer_persistence_spec.rb

+require 'spec_helper'
+
+if defined?(NoBrainer::Document)
+  require 'nobrainer'


the require seems unecessary, as NoBrainer::Document is already defined (so the require is already done)

nviennot · 2018-01-12T15:13:55Z

spec/unit/persistence/no_brainer_persistence_spec.rb

+
+    after do
+      NoBrainer.purge!
+    end


From the describe to here, All this should be global to the rspec test (in rspec_spec? or rspec_spec_nobrainer?), not per test file

Also, consider doing the purge! in config.before(:each) { }, not after. This is because if the previous test crashes or hangs for whatever reason, the after block won't be run.

Well then it's conflicting with your previous comment:

Things that are not specific to NoBrainer should probably be in a separate pull request. [...]

I would have to update the spec/unit/persistence/mongoid_persistence_spec.rb, spec/unit/persistence/mongoid_persistence_mutliple_spec.rb, spec/unit/persistence/no_brainer_persistence_spec.rb and spec/unit/persistence/no_brainer_persistence_mutliple_spec.rb files.

nviennot · 2018-01-12T15:18:33Z

spec/spec_helpers/nobrainer.rb

+
+  rethinkdb_url = "rethinkdb://#{ENV['RETHINKDB_HOST'] || 'localhost'}:"
+  rethinkdb_url << "#{ENV['RETHINKDB_PORT'] || 28015}/"
+  rethinkdb_url << "#{ENV['RETHINKDB_DB'] || "nobrainer_#{Process.pid}"}"


Don't use the Process.pid. Use "aasm_test" instead. This is because are using a new database every time you run the test, resulting in a lot of garbage that won't get cleaned up.

Also if you do this:

NoBrainer.configure do |config| config.app_name = :aasm config.environment = :test end

You should be good. NoBrainer will figure out how to use the ENV variables

nviennot · 2018-01-12T15:19:10Z

spec/spec_helpers/redis.rb

 begin
  require 'redis-objects'
-  puts "redis-objects gem found, running Redis specs \e[32m#{'✔'}\e[0m"


Things that are not specific to NoBrainer should probably be in a separate pull request. Same for Mongoid, and Sequel.

@anilmaurya do you want a spearated PR for those changes?

@zedtux It make sense to send separate PR for changes which are not specific to NoBrainer.

Thank you @nviennot for helping in code review. I appreciate your review. 🏅

Okay, then I will split this part.

nviennot · 2018-01-12T15:20:55Z

lib/aasm/persistence/no_brainer_persistence.rb

+        end
+
+        def aasm_update_column(attribute_name, value)
+          send("#{attribute_name}=", value)


Use write_attribute(name, value) (which does the send() under the hood), but it's more readable and consistent with the other function.

nviennot · 2018-01-12T15:32:27Z

lib/aasm/persistence/no_brainer_persistence.rb

+        def aasm_ensure_initial_state
+          AASM::StateMachineStore.fetch(self.class, true).machine_names.each do |state_machine_name|
+            attribute_name = self.class.aasm(state_machine_name).attribute_name.to_s
+            if self.class.fields.stringify_keys.keys.include?(attribute_name) && send(attribute_name).blank?


This is a bit horrendous. I'm looking at the other ORMs implementation, and checking if the attribute exists is not done for others (except Mongoid). It actually would be better to have an error if the attribute is not declared.

I would just do aasm(state_machine_name).entire_initial_state if read_attribute(attribute_name).blank? (like Dynamoid or Redis)

I fully agree with you. I have to admit I hardly followed what was done with Mongoid which wasn't a good idea for everything. Thank you! 👍

nviennot · 2018-01-12T15:35:27Z

lib/aasm/persistence/no_brainer_persistence.rb

+      module ClassMethods
+        def aasm_create_scope(state_machine_name, scope_name)
+          scope_options = lambda {
+            send(


the send() seems uncessary. Do like CoreDataQuery: lambda { where(...) }

zedtux · 2018-01-16T07:13:59Z

Code updated and tests passed, thank you @nviennot

zedtux · 2018-01-17T06:46:48Z

@anilmaurya the code has been updated

anilmaurya · 2018-01-21T18:10:47Z

@zedtux Could you resolve conflicts and update this PR ?

* Add persistence for NoBrainer * Add tests for NoBrainer * Show the ORM version when running the tests * Add a Dockerfile and a docker-compose.yml file allowing to run from anywhere very easily the test suite with `docker-compose run --rm aasm\' * Moves the Mongoid database name definition in the gem loader (avoiding code repetition) * Update the CHANGELOG.md and the README.md files * Add Rails generator for NoBrainer

zedtux · 2018-01-22T07:46:31Z

@anilmaurya it's done but I don't understand why the jruby version is failing. Can you please guide me?

I mean I don't understand why with Ruby it is working fine, but with jruby it is failing stating that the sqlite3 gem is missing.

anilmaurya · 2018-01-24T07:01:47Z

@zedtux Thank you for updating.
I will try to spare some time for debugging this issue.

zedtux · 2018-01-29T13:18:52Z

@anilmaurya if you need anything from me, please feel free to ask.

anilmaurya · 2018-01-29T13:46:09Z

@zedtux Can you help in fixing travis build for this PR 😊

Thank you for asking ♡

anilmaurya · 2018-02-03T04:32:25Z

Hi @zedtux ,

I am trying to run test suite through docker
It gives me following error:

docker-compose run --rm aasm
Pulling bundle (aasm/aasm:latest)...
ERROR: pull access denied for aasm/aasm, repository does not exist or may require 'docker login'

zedtux · 2018-02-03T14:38:40Z

You have to run first docker-compose build aasm.
Otherwise Docker tries to download the image from Docker hub, but I will let you manage the upload and the maintenance.

anilmaurya · 2018-02-13T07:21:35Z

Thank you @zedtux for your contribution.

zedtux · 2018-02-13T07:52:16Z

Oh that’s great 👍🏻

alto · 2018-02-13T09:24:30Z

@zedtux Great OR. Cheers!

@anilmaurya Thanks for taking care of this!

zedtux mentioned this pull request Jan 12, 2018

Non-UTC timezone issue NoBrainerORM/nobrainer#247

Open

nviennot reviewed Jan 15, 2018

View reviewed changes

zedtux mentioned this pull request Jan 22, 2018

RethinkDB/NoBrainer compatibility? #521

Closed

anilmaurya merged commit 65073e3 into aasm:master Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Nobrainer (RethinkDB) #522

Add support for Nobrainer (RethinkDB) #522

zedtux commented Jan 12, 2018 •

edited

zedtux commented Jan 12, 2018

anilmaurya commented Jan 12, 2018

nviennot Jan 12, 2018

nviennot Jan 12, 2018

nviennot Jan 12, 2018

zedtux Jan 16, 2018

nviennot Jan 12, 2018

nviennot Jan 12, 2018

zedtux Jan 16, 2018

anilmaurya Jan 16, 2018

zedtux Jan 17, 2018

nviennot Jan 12, 2018

nviennot Jan 12, 2018

zedtux Jan 16, 2018

nviennot Jan 12, 2018

zedtux commented Jan 16, 2018

zedtux commented Jan 17, 2018

anilmaurya commented Jan 21, 2018

zedtux commented Jan 22, 2018 •

edited

anilmaurya commented Jan 24, 2018 •

edited

zedtux commented Jan 29, 2018

anilmaurya commented Jan 29, 2018

anilmaurya commented Feb 3, 2018

zedtux commented Feb 3, 2018

anilmaurya commented Feb 13, 2018

zedtux commented Feb 13, 2018

alto commented Feb 13, 2018

Add support for Nobrainer (RethinkDB) #522

Add support for Nobrainer (RethinkDB) #522

Conversation

zedtux commented Jan 12, 2018 • edited

zedtux commented Jan 12, 2018

anilmaurya commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zedtux commented Jan 16, 2018

zedtux commented Jan 17, 2018

anilmaurya commented Jan 21, 2018

zedtux commented Jan 22, 2018 • edited

anilmaurya commented Jan 24, 2018 • edited

zedtux commented Jan 29, 2018

anilmaurya commented Jan 29, 2018

anilmaurya commented Feb 3, 2018

zedtux commented Feb 3, 2018

anilmaurya commented Feb 13, 2018

zedtux commented Feb 13, 2018

alto commented Feb 13, 2018

zedtux commented Jan 12, 2018 •

edited

zedtux commented Jan 22, 2018 •

edited

anilmaurya commented Jan 24, 2018 •

edited