Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor truncation to handle class eval only loaded adapters #221

Closed
wants to merge 7 commits into from

Conversation

tommeier
Copy link
Contributor

Hi,
I've had an attempt at the first cut for running with proper decoration of adapters for truncation.

Though the more I got into it the more design decisions I noticed, and didn't quite understand.

Could you have a look at this? and let me know if you think its on the right track or if i've missed a critical design choice in the cull. The main change is applying on the underlying adapter rather than the superclass (if class_evaling on the superclass, you'd then need to forcibly reload the subclass, or so I can gather). In theory, this should work for the Arjdbc adapters too, but I don't have a running JRuby app to do extensie testing with. Best to get this working with the minimum as so, and work out the other edge cases again, unless you see them straight away.

Though, my moped tests still fail (connection error), but it appears others have that #215 and it probably just needs a 'testing' blurb added to the README. Are the features meant to pass? Or is rake spec the only focus right now?

TODO

  • Test on JRuby with ArJdbc adapters
  • Test with issues on postgres/mysql combination exposed by @simi and @etagwerker
  • Tested on postgres
  • Tested on mysql
  • Tested on mongo
  • Tested on couchpotato
  • Tested on Sqllite3 (thanks @fredwu )
  • Tested on redis
  • Fix issues with datamapper
  • Update README with 'how to run test setup' (specifically couchpotato and moped connection issues)

@bmabey
Copy link
Contributor

bmabey commented Jun 12, 2013

Thanks @tommeier for your work on this. What you've done so far looks good, but like you said there are a lot of edgecases to look for. @simi and @etagwerker, it would be great if you could try out this branch to see if it resolves your concerns.

WRT the features... I need to invest some more time in getting them back in working order, running on travis, and perhaps providing a Vagrantfile for people to test the features if they want to (ideally travis would be enough for most people). I've opened issue #222 to remind me to do this.

@parndt
Copy link

parndt commented Jul 15, 2013

We're using this fork/branch in globalize3's rails4 branch and it seems to be working alright

@bmabey
Copy link
Contributor

bmabey commented Jul 15, 2013

Great. @tommeier, what issues with DataMapper are you running into exactly?

@tommeier
Copy link
Contributor Author

No issue, I just don't have an active app to test it properly against,

On 16/07/2013, at 2:40 AM, Ben Mabey notifications@github.com wrote:

Great. @tommeier, what issues with DataMapper are you running into exactly?


Reply to this email directly or view it on GitHub.

@bmabey
Copy link
Contributor

bmabey commented Jul 15, 2013

Ok, I just asked because you had a Fix issues with datamapper checkbox. You also have a checkbox about updating the README.. your changes don't appear to change the API so what would need to be updated?

@bmabey
Copy link
Contributor

bmabey commented Jul 15, 2013

nm, I see it is just about updating the README on how to run tests which is orthogonal to this change and can be in a different PR.

@tommeier
Copy link
Contributor Author

I think I was having issues with DataMapper, but looks ok to me now, I'd really hope someone with working moped, couchpotato and most importantly a JRuby implementation to test against, but everything seems to be ok for the others.

@tommeier
Copy link
Contributor Author

I reckon this is ok, if it goes into master, at least anyone on couchpotato etc (that i haven't been able to test), can point out any issues with it?

@fredwu
Copy link
Contributor

fredwu commented Aug 22, 2013

Tested on sqlite3 memory DB and no issues! 👍

@tommeier
Copy link
Contributor Author

Merge of master brought in some clutter, i've created off current (1.1.1) code and recreated pull request: #241

@tommeier tommeier closed this Aug 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants