remove one more Mongoid dependency when rescuing from _invoke #54
Conversation
alexagranov
force-pushed
the
no_mongoid_exception_on_activerecord
branch
7 times, most recently
from
b83b935
to
eeec1e1
Compare
|
Do you think you could try and add a test for this? There should be a way to raise a validation error and get a failure in the AR version. |
|
Ok. I can add for AR, just realized the specs are using |
| if defined?(::Mongoid) | ||
| def invoke(client, data) | ||
| _invoke client, data | ||
| rescue Mongoid::Errors::Validations => e |
There was a problem hiding this comment.
We really should refactor this out of here, but I am not sure how or where. I am OK with this change for now, just thinking out loud.
There was a problem hiding this comment.
ya, maybe the wrapping of invoke needs to be done in DB-specific dirs under ext/?
alexagranov
force-pushed
the
no_mongoid_exception_on_activerecord
branch
2 times, most recently
from
0a18a45
to
7283748
Compare
|
Just pushed up a spec. This should handle the StandardError case at least. Not sure if Btw, noticed locally that selenium tests fail as I'm on FF 52 and selenium-webdriver is pinned to 2.x series? Is there a reason not to move up to 3.3.0? |
alexagranov
force-pushed
the
no_mongoid_exception_on_activerecord
branch
2 times, most recently
from
dbd08c2
to
4d1c38b
Compare
|
Ok this is great, now add a mongoid spec? You can raise a validation error by creating a dummy model with a Any version of Selenium works as long as specs pass green here, but it's a permanent struggle of upgrading versions all around. |
|
Sounds good but that may take a little longer - I don't want to put Mongo on this box so would need to rely on Travis or a docker VM. What do you think of And yep, I hear ya on Selenium, was just wondering if its a Travis limitation perhaps but I just double-checked and Travis can pull down latest FF. |
|
Re: separate version locations works for me. Btw you can download mongo and start it temporarily, it's self contained. |
|
Oh ya? Shazam. I don't remember that, let me give it a whirl... |
alexagranov
force-pushed
the
no_mongoid_exception_on_activerecord
branch
from
4d1c38b
to
0a3fc20
Compare
|
Ok, Mongoid validation exception handling spec added. Along with splitting the That self-contained binary is handy, I didn't want to mess with homebrew 🍻 |
| require_relative "slack-ruby-bot/#{ext}" | ||
| end | ||
| require_relative 'slack-ruby-bot/client' | ||
| require_relative 'activerecord/slack-ruby-bot/commands/base' if defined?(::ActiveRecord) |
There was a problem hiding this comment.
This should be require_relative "{SlackRubyBotServer::Config.database_adapter}/slack-ruby-but/commands/base" which reads cleaner and is forward compatible for other adapters.
There was a problem hiding this comment.
Ah yes indeed, will do.
|
See minor comment above, almost there. Thanks! |
spec/ext/commands_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
| if SlackRubyBotServer::Config.mongoid? |
There was a problem hiding this comment.
I would refactor this into a separate spec, maybe with a directory structure similar to the code, even if it means a bit of duplication, for future adapter-type variations.
There was a problem hiding this comment.
True, coming up.
alexagranov
force-pushed
the
no_mongoid_exception_on_activerecord
branch
from
0a3fc20
to
656e53c
Compare
|
OK, updated with suggestions. There was some difficulty with conditionally loading the DB-specific specs from I wasn't able to use just use I hope you're ok with adding the exclusion pattern in Rakefile to avoid any specs under |
| @@ -5,7 +5,7 @@ require 'rspec/core' | |||
| require 'rspec/core/rake_task' | |||
|
|
|||
| RSpec::Core::RakeTask.new(:spec) do |spec| | |||
| spec.pattern = FileList['spec/**/*_spec.rb'] | |||
| spec.pattern = FileList['spec/**/*_spec.rb'].exclude(%r{ext\/(?!#{ENV['DATABASE_ADAPTER']})}) | |||
|
Perfect, merged. |
|
@alexagranov Want to help with this library? Maybe make a release? I would be very happy to invite you to contributors and give you rubygems access (what's your rubygems email?). |
|
@dblock Sure, I can lend a hand. If all goes well I'll be leveraging this library quite a bit... I use the same email as my GitHub account for Rubygems: alex@morphogenic.net |
|
You're in @alexagranov! |
No description provided.