Sequel adapter #152

appleton · 2015-04-04T17:04:10Z

I'm sure this needs more work, specifically there seems to be a fair bit of behaviour in the ActiveRecord adapter only which would also be applicable here, but I'm opening a PR to get some discussion going.

I'm thinking I could factor out a lot of the active record adapter specs into another shared suite, something like it_behaves_like "a SQL adapter".

I guess it would also be useful to implement the queries interface.

greysteil · 2015-04-07T21:27:15Z

This looks really nice @appleton. Agree it would be really nice to add the queries interface - would also be good to use a most_recent column to make that fast (like we do with ActiveRecord).

greysteil · 2015-04-07T21:27:37Z

lib/statesman.rb

@@ -11,6 +11,7 @@ module Adapters
             "statesman/adapters/active_record_transition"
    autoload :ActiveRecordQueries,
             "statesman/adapters/active_record_queries"
+    autoload :Sequel, "statesman/adapters/sequel"


What's going on with the whitespace here?

petehamilton · 2015-11-26T14:14:56Z

@appleton hey hey 👋 We're thinking ahead to a 2.0 release this side of Christmas, feels like this would be a nice PR to get in? Is there anything left to do on it before it can be merged?

appleton · 2015-11-26T15:23:53Z

I think if the tests pass then it should be good to go though we're not using this in any project yet so I can't speak to its stability. We are just considering pulling statesman into a project though so it could be good timing :)

appleton · 2015-11-26T15:25:04Z

Well, that said there's no query interface but maybe its okay for that to come later?

petehamilton · 2015-12-07T10:23:57Z

Sorry for the delay in getting back to you! Given this isn't a breaking change & there's no real rush on getting it in, I wonder if it's worth waiting until you're using it in production? What do you think?

appleton · 2015-12-07T10:29:20Z

Works for me!

On 7 December 2015 at 10:24, Pete Hamilton [email protected] wrote:

Sorry for the delay in getting back to you! Given this isn't a breaking
change & there's no real rush on getting it in, I wonder if it's worth
waiting until you're using it in production? What do you think?

—
Reply to this email directly or view it on GitHub
#152 (comment).

greysteil · 2016-02-28T18:22:13Z

@appleton - any update? Would love to get this in if you've battle-tested it!

appleton · 2016-03-01T17:18:24Z

It's not battle tested, we backed out of the change we were planning to make unfortunately. I'd suggest one of:

Close this PR and forget about it for ever
Merge it and mark it as experimental in the docs (maybe even add some feature flag in the code?)
Extract it into its own gem. I think it actually makes more sense to say statesman is written for Active Record but adapters exist separately for other ORMs. This would also let you off the hook for maintaining the Mongo adapter why doesn't get much love :)

On 28 Feb 2016, at 6:22 pm, Grey Baker [email protected] wrote:

@appleton - any update? Would love to get this in if you've battle-tested it!

―
Reply to this email directly or view it on GitHub.

greysteil · 2016-03-02T18:40:03Z

I'm for extracting into a gem - this isn't going to get the same level of support as the rest of Statesman, and I don't want it to slow down development. @hmarr / @petehamilton - what do you reckon?

hmarr · 2016-03-03T17:38:54Z

My preference would also be extracting into a gem. Seems like a sensible approach for alternative adapters in general.

badosu · 2016-03-09T14:04:22Z

@appleton I am looking for a state machine gem and I am very in favour of this one. However tha lack of Sequel support is weighting in.

Apart from the query interface, is this adapter working as it should with statesman? If so, I can fork your adapter and help extracting it into a gem.

appleton · 2016-03-09T14:07:35Z

Hey @badosu yeah it should be working just fine, it passes the adapter tests at the very least. I'd be happy to help out with advice or possibly contributions if you want to take it on, I think it's a great idea!

badosu · 2016-03-09T14:23:28Z

@appleton Thanks for the quick response.

We are going to implement a state machine on our system in the next weeks, I will update you when we come back to this.

badosu · 2016-04-18T21:11:08Z

lib/statesman/adapters/sequel.rb

+
+      ::Sequel.extension(:inflector)
+
+      def initialize(transition_class, parent_model, observer)


This method now requires an additional options={} argument to work on the latest version.

badosu · 2016-04-18T21:14:16Z

@appleton I will start using your code on the next few days, I'll post my findings and refine the adapter as it will be battle tested on the next weeks.

badosu · 2016-04-18T21:49:21Z

@greysteil @hmarr Is there any reason for preferring using null values on the most_recent column when nullable? https://github.com/gocardless/statesman/blob/master/lib/statesman/adapters/active_record.rb#L100

I am developing this specific part for the adapter and this adds considerable complexity, I am not sure of the benefit. Thanks!

greysteil · 2016-04-19T07:58:58Z

It was a requirement for people adding a most_recent column to an existing state machine - i.e., upgrading from Statesman 1.1 to 1.2 (version numbers may be wrong).

Given that the sequel adapter is new, there's no need to support that upgrade path, and you don't need to worry about the most_recent column ever being nullable.

badosu · 2016-04-19T19:52:03Z

@greysteil Great! Thanks for the follow up.

badosu · 2016-12-20T16:12:01Z

@greysteil @appleton We finally can attest for sure that the code is running fine on production for months and feel confident to contribute the changes.

We can update this PR or create a gem for this: sequel-statesman or statesman-sequel.

@appleton Let me know how it works best for you, since if we publish this in a separate repo we'll lose your commit history, but I'd like to make certain that your contribution credit is not lost.

appleton · 2016-12-20T16:53:34Z

Hey @badosu, glad to hear it's working! Please do take it and create your own project, I'm fine with losing the attribution :)

greysteil · 2016-12-20T18:51:21Z

@hmarr can you or someone from GC make the call on PR versus separate gem? Latter makes sense to me but I'm the world's worst maintainer whilst I'm on the road.

hmarr · 2016-12-20T19:40:41Z

I'd rather go with a separate gem so there's clear ownership. Having the Mongo adapter in the main repo can make it harder to work on Statesman, as most of the core contributors aren't that familiar with Mongo. Moving future adapters to separate gems seems like a sensible approach.

Thanks for pushing this forward @badosu!

badosu · 2016-12-20T19:43:12Z

Thanks @hmarr, @greysteil, @appleton!

Packaging it into a gem will also help in regard to inclusion of a sequel plugin. That would require a separate gem from statesman anyways.

I'll report back as soon as I finish polishing the gem.

badosu · 2016-12-21T18:12:54Z

@appleton, @hmarr, @appleton

I just released a very preliminar version at https://github.com/badosu/statesman-sequel

I'll consider it ready for use when I have migrated all the adapter specs to my suite. For now I am testing the basic behaviour with the plugin included in the gem.

Thanks for everyone!

hmac · 2017-05-26T11:25:12Z

As of #250 we'll link to that in the README - thanks!

appleton added 3 commits March 14, 2015 16:21

Initial Sequel adapter

3fb1315

Failing shared specs for sequel adapter

882e854

Implement shared behaviour for Adapters::Sequel

cb97407

greysteil reviewed Apr 7, 2015
View reviewed changes

Separate shared SQL adapter behaviour specs

096c7e4

appleton force-pushed the sequel-adapter branch from 44787ee to 096c7e4 Compare April 9, 2015 08:50

appleton added 2 commits April 9, 2015 09:52

Align everything, autoload SequelTransition

8be167d

Use reflection to get a more accurate foreign key

fb01c84

badosu reviewed Apr 18, 2016
View reviewed changes

Sinjo mentioned this pull request May 26, 2017

Start list of notable third-party extensions #250

Merged

hmac closed this in #250 May 26, 2017

hmac deleted the sequel-adapter branch May 26, 2017 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sequel adapter #152

Sequel adapter #152

appleton commented Apr 4, 2015

greysteil commented Apr 7, 2015

greysteil Apr 7, 2015

petehamilton commented Nov 26, 2015

appleton commented Nov 26, 2015

appleton commented Nov 26, 2015

petehamilton commented Dec 7, 2015

appleton commented Dec 7, 2015

greysteil commented Feb 28, 2016

appleton commented Mar 1, 2016

greysteil commented Mar 2, 2016

hmarr commented Mar 3, 2016

badosu commented Mar 9, 2016

appleton commented Mar 9, 2016

badosu commented Mar 9, 2016

badosu Apr 18, 2016

badosu commented Apr 18, 2016

badosu commented Apr 18, 2016

greysteil commented Apr 19, 2016

badosu commented Apr 19, 2016

badosu commented Dec 20, 2016 •

edited

Loading

appleton commented Dec 20, 2016

greysteil commented Dec 20, 2016

hmarr commented Dec 20, 2016

badosu commented Dec 20, 2016 •

edited

Loading

badosu commented Dec 21, 2016

hmac commented May 26, 2017


		::Sequel.extension(:inflector)

		def initialize(transition_class, parent_model, observer)

Sequel adapter #152

Sequel adapter #152

Conversation

appleton commented Apr 4, 2015

greysteil commented Apr 7, 2015

greysteil Apr 7, 2015

Choose a reason for hiding this comment

petehamilton commented Nov 26, 2015

appleton commented Nov 26, 2015

appleton commented Nov 26, 2015

petehamilton commented Dec 7, 2015

appleton commented Dec 7, 2015

greysteil commented Feb 28, 2016

appleton commented Mar 1, 2016

greysteil commented Mar 2, 2016

hmarr commented Mar 3, 2016

badosu commented Mar 9, 2016

appleton commented Mar 9, 2016

badosu commented Mar 9, 2016

badosu Apr 18, 2016

Choose a reason for hiding this comment

badosu commented Apr 18, 2016

badosu commented Apr 18, 2016

greysteil commented Apr 19, 2016

badosu commented Apr 19, 2016

badosu commented Dec 20, 2016 • edited Loading

appleton commented Dec 20, 2016

greysteil commented Dec 20, 2016

hmarr commented Dec 20, 2016

badosu commented Dec 20, 2016 • edited Loading

badosu commented Dec 21, 2016

hmac commented May 26, 2017

badosu commented Dec 20, 2016 •

edited

Loading

badosu commented Dec 20, 2016 •

edited

Loading