Updated shared adapter spec to be more configurable #119

Merged
merged 8 commits into from Jan 8, 2012

Projects

None yet

4 participants

@mbj
Owner
mbj commented May 29, 2011

The intention is to use the shared adapter specs from
dm-mongo-adapter without the need to duplicate it.

Tested against sqlite-adapter mongo-adapter and in-memory adapter, on
ruby-1.9.2-p180.

Markus Schirp added some commits May 29, 2011
Markus Schirp Updated shared adapter spec to be more configurable
The intention is to use the shared adapter specs from
dm-mongo-adapter without the need to duplicate it.

Tested against sqlite-adapter mongo-adapter and in-memory adapter, on
ruby-1.9.2-p180.
eb8cdba
Markus Schirp fixed stupid typo 6b29508
Contributor
solnic commented May 31, 2011

That's what I wanted to do a long time ago although I was thinking about doing a small refactor in those shared specs so they would be using a model which is provided by the adapter spec or defaulting to Heffalump if nothing is provided. I think that would be cleaner than setting up resource module and serial property ivars. 99% of the adapters don't require you to include a different module than DM::Resource and serial property will be gone sooner or later and replaced by some generic object id property.

Owner
mbj commented May 31, 2011

I agree that the use of a Adapter specific Heffalump where needed is a better solution. (As long it has compatible properties).

The shared adapter spec is currently configured by ivars set in before hooks (@adapter,@repository) so I did configure the Heffalump with ivars too.

This pull request preserves the default behavior on adapters who do not set the @dm_resource_module and @dm_resource_property for shared adapter specs. I do not see a reason not to use this solution in the meantime?

Contributor
solnic commented May 31, 2011

We're moving away from using ivars in specs and prefer to use let/let! from rspec when setting up objects required by a spec example. You could refactor the adapters shared specs if you have time. If you don't, I can take care of that :)

Owner
mbj commented May 31, 2011

I'll configure this with the let/let!, stay tuned.

Owner
mbj commented Jun 1, 2011

I got some thoughts on the changes:

The adapter specific model must have the "same" properties like the Heffalump. Else the shared adapter test cannot create/update resources etc in a reliable way.

The model has to be assigned to a constant, else DataMapper.finalize! will fail. (#name will be nil).

In case no let or let! block does configuration the "default" Heffalump (or any other option) will be used. The absence of a let/let! is only detectable with respond_to?(:name_of_block).

Any suggestions? I would appreciate your review on my changes once they are pushed to github. If the changes are okay I'll try to nuke the ivars from the sqlite and postgres shared adapter specs also. I cannot run specs against the other adapters, I do not have the other databases in house.

Owner
dkubb commented Aug 16, 2011

This looks good to me. @solnic is this what you were looking for?

Owner
mbj commented Aug 16, 2011

The Heffalump does not have to be a constant. It can be an anonymous class as long as you override #name.

Example:

let(:heffalump) do
  Class.new do
    include DataMapper::Resource
    property :id,DataMapper::Mongo::Property::ObjectId
    def self.name; 'Heffalump'; end
  end
end

Doing heffalump setup this way would remove the need to unset constants. Suggestions?

Owner
dkubb commented Aug 16, 2011

I actually do this in my own specs now. I don't like the idea of creating a constant unless there's no other choice (which is rarely the case). I would support this as a change to the shared specs.

@dkubb since I'm working on the shared specs too should I incorporate this proposal when working on the capabilities API? https://gist.github.com/1154417

Owner
dkubb commented Aug 18, 2011

@jeroenvandijk I think it would be ok to specify somewhere in the proposal.

However, I don't think this issue should wait for your work specifically, which is more overarching that this small change that @mbj proposes. If he wants to submit a patch to this pull request that uses an anonymous class I think he shouldn't be prevented. We can iterate on his code in your patches if needed.

Owner
mbj commented Aug 19, 2011

In case the Heffalump model is created using let. Is it still overridable? This was my main intend to work at this.

Contributor
solnic commented Sep 13, 2011

@mbj sorry for late response. yes, you can override things by another #let declarations.

btw - let's get this merged in for 1.3.0

Owner
mbj commented Sep 13, 2011

@solnic, No worries. Once we got that merged dm-mongo-adapter might become a good dm-1.0 citizen ;)

Contributor
solnic commented Oct 15, 2011

@mbj can we merge this in? I'm not sure if this still works with other adapters, please confirm

Owner
mbj commented Oct 15, 2011

@solnic, Im unsure. Will have to test for myself. Im time limited atm. Give me some days.

Owner
mbj commented Jan 4, 2012

IMHO this is mergable now.

See also:

datamapper/dm-do-adapter#12
datamapper/dm-sqlite-adapter#5
https://github.com/mbj/dm-mongo-adapter/tree/feature/general-shared-adapter-spec

@solnic the dm-mongo-adapter branch will be merged with master once dm-core supports overriding the default Heffalump. IMHO we will be able to release a beta gem along with dm-1.3

I do not have a local postrgres, mysql or other -sql database anymore and I avoid to make pull requests for untested changes.

@solnic solnic merged commit 64bbcc6 into datamapper:master Jan 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment