Comments for sam's dm-more  

This actually causes a major issue for Windows users as bcrypt-ruby doesn’t have a Windows version of the gem.

- Matt

I would suggest making the part of the spec where you check stuff like “@mj.height.should == 0.0” with “@mj.should be_valid” into a separate “it” block. The state of the attribute is a separate observable fact from the object being valid.

I’ve sent a pull request for a commit that cleans this up a bit to avoid the regex match; see http://github.com/gcampbell/dm-more/commit/b7128768cf9649f11b477f7ba1107869b742a800

@gotfat: duh. sorry it’s late here. I see now what the intention was. I agree with your suggestion.

In general matching on .* seems a bit dangerous. I would suggest using something like your suggestion, perhaps even with ^ or \A at the beginning of the regexp to anchor the match to tbe beginning of the string.

A nicer solution would be to see if nokogiri and libxml can skip producing a prologue. If there’s no supported option now, perhaps it’s something we could request?

I thought it was stripping:

<?xml version=“1.0” encoding=“UTF-8”?>

or friends, but it didn’t really strip it. If it was that case, perhaps:

sub(/<\?.*?\?>/

would be better.

If your intention is to strip off all trailing whitespace just use String#rstrip.

Actually, I’m responsible for converting dm-types to use autoload, but as the merb guys have discovered, autoload isn’t thread safe, so I’m not sure if it will remain that way forever.

I suppose it’s good to question the likelihood that two threads will be defining models at the same time, thus requiring thread safety. I’m not sure, but I think it’s worth discussing (perhaps on the mailing list?).

At the same time, with us wanting to target JRuby in a future release, this may cause problems for us. Is there a way to say that bcrypt-ruby should only be a dependency for MRI?

I don’t think bcrypt-ruby should be a dependency b/c it’ got c extensions. It’s protected by an autoload, so if you don’t use an encrypted type, it will never try to require bcrypt.

This spec passes on my system without the pending wrapper. Did it fail on yours? With the wrapper I get a blue F with the following error message:

‘DataMapper::Validate::ValidatesWithBlock should validate via a block on the resource’ FIXED Expected pending ‘Fix error conetxt name’ to fail. No Error was raised.

If it’s in the repository I’d like to package it into the gem. I don’t want to have any exceptions where someone always has to remember to exclude certain files when they run check_manifest prior to releasing the gem. It’s hard enough keeping track of ~30 gems, nevermind what they might include or exclude.

benchmarks are a bit hacky, are they really needed in the gem?

Yes, those benchmarks match what I was seeing as well – that’s why I put LibXML ahead of Nokogiri on the “fallback order” list.

Nokogiri is a fraction slower but they’re the same order of magnitude

REXML
Running the benchmarks 2000 times each...

                               Results |
----------------------------------------
Serialization: Single Resource    2.75 |
                    Collection    3.48 |
----------------------------------------
Nokogiri
Running the benchmarks 2000 times each...

                               Results |
----------------------------------------
Serialization: Single Resource    0.55 |
                    Collection    0.99 |
----------------------------------------
Running the benchmarks 2000 times each...

                               Results |
----------------------------------------
Serialization: Single Resource    0.46 |
                    Collection    0.87 |
----------------------------------------

What’s the speed difference between LibXML and Nokogiri?

Hehe, we should probably rename the module to Timestamps, and alias that back to Timestamp. I wasn’t aware of the naming inconsistency between the module and the gem name.

Ahh interesting. I could swear I tested this once and it returns an empty Array. I will fix the code since I don’t like iterating over an Array of nil values like that.

>> [(1 if false), (2 if false)]
=> [nil, nil]

I seem to remember there was a good reason for the rescue, but I can’t recall now. Hoping it wasn’t laziness :(

It’s not a big deal – the code is clear.

Since the “if” condition code is wrapped up in parenthesis, if none of the Proxy classes are defined (as in dkubb/dm-core) the Array should be empty, so the each call would never iterate over anything.

I’m not sure why there’s a rescue there though. I think it would probably be better to write it so that it only undefined the method if it existed.

to be “correct” I think the array needs to be compacted, nil.send(:undef_method) just raises (and is caught).

+1 for bernerd’s change.