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

Added Conditions to activerecord #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielgranat
Copy link
Contributor

I followed your advice on how to add it, and its done.
You can see a test that adds 4 Seets to the Car.
2 are front seets,
2 back seets,
1 seet from the front is the drivers seet. This one is only added in code to the front seet, but pulled form the DB as driver (using conditions)

LMK what you think, and if there is anything else that needs to be added/removed

Daniel

…t all test pass. Next to actually test the conditions
Test cover conditions in form of:
1. Array
2. Hash
3. String
@jwood
Copy link
Owner

jwood commented Aug 19, 2011

Hi Daniel,

Looks good. This functionality will also need to be added to the DataMapper and Sequel adapters as well. Any interest in looking into that? I'm busy right now with some other projects, so it may take me a while to make that change.

John

@danielgranat
Copy link
Contributor Author

That is odd... i see the change on my FS but git shows nothing that's changed..
I will update you when this is fixed

On Fri, Aug 19, 2011 at 2:55 PM, jwood
[email protected]
wrote:

Hi Daniel,

Looks good.  This functionality will also need to be added to the DataMapper and Sequel adapters as well.  Any interest in looking into that?  I'm busy right now with some other projects, so it may take me a while to make that change.

John

Reply to this email directly or view it on GitHub:
#30 (comment)

@danielgranat
Copy link
Contributor Author

My mistake, i was looking at the master branch.
Your intention is that both DataMapper and Sequel should use the
condition and not just change the function interface?

I might have some time to do that..

Daniel

On Fri, Aug 19, 2011 at 4:59 PM, Daniel Granatshtain
[email protected] wrote:

That is odd... i see the change on my FS but git shows nothing that's changed..
I will update you when this is fixed

On Fri, Aug 19, 2011 at 2:55 PM, jwood
[email protected]
wrote:

Hi Daniel,

Looks good.  This functionality will also need to be added to the DataMapper and Sequel adapters as well.  Any interest in looking into that?  I'm busy right now with some other projects, so it may take me a while to make that change.

John

Reply to this email directly or view it on GitHub:
#30 (comment)

@jwood
Copy link
Owner

jwood commented Aug 19, 2011

Correct. Ideally this setting would be used by any of the SQL based libraries.

@danielgranat
Copy link
Contributor Author

Make sense. i wasn't aware of any other SQL based extensions.
Should it be any different then the activerecord one? Just add the
conditions in the same way?

On Fri, Aug 19, 2011 at 5:10 PM, jwood
[email protected]
wrote:

Correct.  Ideally this setting would be used by any of the SQL based libraries.

Reply to this email directly or view it on GitHub:
#30 (comment)

@jwood
Copy link
Owner

jwood commented Aug 19, 2011

The implementation will need to be slightly different for the other library extensions. The ActiveRecord implementation uses sanitize_sql to build the conditions, but that is implemented in ActiveRecord::Base. We'll need to find a way of doing this specific to each library, since ActiveRecord may not be loaded if somebody is using DataMapper and Mongoid, for example.

@jwood
Copy link
Owner

jwood commented Aug 19, 2011

Oh...one other thing Daniel. I noticed a spelling error in the code. It's "seat" instead of "seet". If you're in the code adding support for the other libraries, would you mind correcting this?

@danielgranat
Copy link
Contributor Author

oops... will do

On Fri, Aug 19, 2011 at 6:59 PM, jwood
[email protected]
wrote:

Oh...one other thing Daniel.  I noticed a spelling error in the code.  It's "seat" instead of "seet".  If you're in the code adding support for the other libraries, would you mind correcting this?

Reply to this email directly or view it on GitHub:
#30 (comment)

@danielgranat
Copy link
Contributor Author

Hi john,
I fixed the Stear thing.

As for the other class, i don't think i will have time to handle that
in the near future.

Daniel.

On Sat, Aug 20, 2011 at 10:34 AM, Daniel Granatshtain
[email protected] wrote:

oops... will do

On Fri, Aug 19, 2011 at 6:59 PM, jwood
[email protected]
wrote:

Oh...one other thing Daniel.  I noticed a spelling error in the code.  It's "seat" instead of "seet".  If you're in the code adding support for the other libraries, would you mind correcting this?

Reply to this email directly or view it on GitHub:
#30 (comment)

@jwood
Copy link
Owner

jwood commented Aug 22, 2011

Ok, thanks Daniel. I'll merge your commit into a topic branch, and continue to work on adding support for the :conditions to the other SQL based libraries when I'm free.

@danielgranat
Copy link
Contributor Author

Thanks John.

This is a project i was working on for a company Pixfizz.
Having the project ended, i will keep them posted of any progress and when
they can move back to working with your code base.

Daniel

On Mon, Aug 22, 2011 at 3:52 PM, jwood <
[email protected]>wrote:

Ok, thanks Daniel. I'll merge your commit into a topic branch, and
continue to work on adding support for the :conditions to the other SQL
based libraries when I'm free.

Reply to this email directly or view it on GitHub:
#30 (comment)

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.

2 participants