search

gratipay / gratipay.com

Here lieth a pioneer in open source sustainability. RIP
https://gratipay.news/the-end-cbfba8f50981
MIT License
1.12k stars 308 forks source link

root out SQL building #405

Closed chadwhitacre closed 11 years ago

chadwhitacre commented 11 years ago

There's a couple places where we build SQL through concatenation. We're starting to look at porting to SQLAlchemy (#129), which, if carried to completion, would take care of all this. In the mean time we should get rid of the couple places where we construct SQL manually. If I'm not mistaken this is just a couple where clauses.

chadwhitacre commented 11 years ago

@cyberjacob @artagnon @justinlilly @lyndsysimon @alexcouper Anyone want to tackle this one?

alexcouper commented 11 years ago

I wouldn't have thought this would be doable before gittip has ported to SQLAlchemy - or did "this one" encapsulate changing over to SQLAlchemy as well?

lyndsysimon commented 11 years ago

@alexcouper I think it would best to go ahead and move to SQLAlchemy on a chunk-by-chunk basis, per comments in #129.

I don't think I'll be able to tackle this, as I've not used SQLAlchemy yet without the declarative syntax. I will be watching the implementation though, and will probably throw in an opinion or two and may join in little bit later :)

alexcouper commented 11 years ago

@lyndsysimon I agree - I guess I'm a little puzzled as to where #405 lives in relation to the end of #129.

@mjallday 's last two comments seemed to suggest that a) he was going to begin the move over in a separate branch and b) he'd be starting by migrating authentication.py

I'm unsure whether there's room for more than one person to do the first step - as someone will want to define centrally the models/table structure - but I could be wrong.

Or has (a) already happened?

chadwhitacre commented 11 years ago

Blech. Okay! Found it: 9168a2e7abcecdede9ebfe6d9284849cbf54d561. The release script assumes that it can safely modify and push gittip/init.py. That wasn't true in this case. Underlying issue ticketed as #410.

lyndsysimon commented 11 years ago

Okay, now I'm really confused. I'm just going to sit back and watch for a while :)

chadwhitacre commented 11 years ago

@alexcouper @lyndsysimon Back to the point at hand, I expect the ORM work to take some doing, and de-replacing SQL might be easier. I've fixed the instance that @mjallday flagged on #129. The other two places we do this are:

https://github.com/whit537/www.gittip.com/blob/3933fabc6fef7fc953cd7fc1cbf3eac9735506b3/gittip/__init__.py#L305

and

https://github.com/whit537/www.gittip.com/blob/3933fabc6fef7fc953cd7fc1cbf3eac9735506b3/gittip/billing/__init__.py#L58 (three instances; search for '%%')

In both it's clearer what's going on, I think. In the authenticate.py case the string replacement was coming in as an argument to the function, so even though it was only called just above it felt a lot worse. In the other two cases there's both a lot more overlap and a lot less being changed.

chadwhitacre commented 11 years ago

Mostly though I'm just looking for good ways to involve you all in contributing. :-)

If there are other tickets that catch your interest then let's talk about 'em! :-)

chadwhitacre commented 11 years ago

I believe stored procedures would be a way to solve this.

chadwhitacre commented 11 years ago

We also do this a few places in test fixture.

alexcouper commented 11 years ago

I'm interested in contributing more - possibly on this ticket, or another if this is done before I have the opportunity.

The SQLAlchemy porting is more of interest to me than changing the way SQL strings are formed TBH.

I'm tied up for the whole of December unfortunately, but I'm pledging 4 full working days in January to gittip. (There should be a site for that kind of OSS pledge). I'll get in touch on the dates I'll be doing that.

chadwhitacre commented 11 years ago

@alexcouper Sweet! :D

I'm going to close this ticket since we're moving forward with #129. Do you use IRC? The #gittip channel on Freenode is a great way to discuss in real-time.

chadwhitacre commented 11 years ago

@alexcouper I just added you as a collaborator on this repo. Welcome aboard. :-)

alexcouper commented 11 years ago

Thanks @whit537! I'm away at the moment but when I get back I'll be joining that IRC channel. FYI the 4 days in Jan look like they'll be spread between 15th and 26th, but I'll confirm closer to the dates.