Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #2236 (closed defect: wontfix)

Opened 10 years ago

Last modified 9 years ago

Uncapitalize DBsession

Reported by: pitrou Owned by: mramm
Priority: normal Milestone: 2.1rc1
Component: Quickstart Templates Version: 2.0b5
Severity: minor Keywords:
Cc:

Description

Sorry, I know this looks like bikeshedding, but since Turbogears 2 is mostly PEP8-compliant, would it be possible to fix DBSession and replace it with something like db_session or dbsession?

Change History

comment:1 Changed 10 years ago by faide

+1 but in a future version because we should not change the API in this stabilization phase...

comment:2 Changed 10 years ago by chrisz

The capitalization of the name has a reason. DBSession is actually not a session, but a (scoped) session constructor, i.e. acting like a class. It only looks like an object because you can call most of its methods on the class level. To avoid confusion, we should follow the SQLAlchemy documentation which also capitalizes all session constructors.

comment:3 Changed 10 years ago by faide

In that case why not call it SAScopedSession, SASessionFactory ? This would be more clear. Even me who knows this is a scoped session (and what a scoped session is) am perturbed by that name.

This is not only a capitalization issue, but also a general naming issue. DBSession whatever the capitalization lets you think it _is_ a session on a db, which it is not.

comment:4 Changed 10 years ago by pitrou

I'd say that practicality beats purity and if it is used like an object, it should probably be named as such. (I suppose calling one of the class methods automatically creates some kind of thread-local object under the hood?)

comment:5 Changed 10 years ago by faide

This is in fact a factory (ie: some kind of object producer) and yes, using the class methods will in fact create a thread local session stored in SA's internal registry and will work as a proxy for the object.

In fact in previous SA versions the session, was just named session and they changed it to a capitalized version in case of scoped session because people did not remark it was indeed a factory.

The DBSession is a class. What I think is that the name is not well chosen because it is not a "db session" but a "db session factory & thread local registry", hence the proposed names.

comment:6 Changed 10 years ago by pitrou

Well, the reason why I opened this bug is that the session (or session factory) is used quite a lot in any model code. Naming it something long and unpractical like SAScopedSession would not work out very well in daily use :-)

comment:7 follow-up: ↓ 11 Changed 10 years ago by chrisz

This notation was already used at least since SA 0.4 (see  here, the Session corresponds to our DBSession).

I'm against using lower case "db_session" name since this will confuse people who are reading the SA docs - they will think it corresponds to a normal "session" instance. On the other hand, I agree with pitrou: "SAScopedSession" is too complicated for a name you're using so frequently. Maybe as a pragmatic compromise, we can add an alias name "db" (but leaving DBSession for compatibility and for purists)? db.query(...) really is much better than anything else. And the name "session" could be reserved to beaker sessions.

comment:8 follow-up: ↓ 9 Changed 10 years ago by faide

Anyway you could always import like this:

from myapp.model import DBSession as dbsession

this would work as intended for you.

I am ok with Chris that the Capitalized name should not go (because of the nature of the object) and I am only arguing on which better capitalized name we could find to this object :)

comment:9 in reply to: ↑ 8 Changed 10 years ago by pitrou

Replying to faide:

Anyway you could always import like this:

from myapp.model import DBSession as dbsession

Yes of course. But there is a lot of generated code which relies on DBSession. (then of course I can create an alias, but it becomes more and more confusing)

comment:10 Changed 10 years ago by mramm

The main thing I care about is that SQLAlcmemy sets this up as Session, so i would like us to maintain as close a connection to that. Also session (lowercase) is a reference to the web session variables, and pylons has Session (SA) and session (beaker). TG2 chose DBSession after quite a bit of discussion a year ago, so I think this boat has sailed.

But, I like python's flexibility so feel free to import it as db if that makes you happy ;)

comment:11 in reply to: ↑ 7 Changed 10 years ago by mramm

Replying to chrisz:

Maybe as a pragmatic compromise, we can add an alias name "db" (but leaving DBSession for compatibility and for purists)? db.query(...) really is much better than anything else. And the name "session" could be reserved to beaker sessions.

This is a reasonable suggestion, and it looks nicer than most of the other options as an API. We could add it as an alias, but the DBSesion and the alias would live in user code, not the framework so this is really a quickstart template discussion...

I'm still +0 on any rename as done is done, and it's not a real Pep-8 problem since DBession IS a class. ;)

comment:12 Changed 10 years ago by jorge.vargas

  • Owner set to Chris Arndt
  • Component changed from TurboGears to Quickstart Templates

+1 on mark's last two comments.

Perhaps a big comment on top of the name explaining a little bit of this?

comment:13 Changed 10 years ago by mramm

  • Milestone changed from 2.0rc1 to 2.1

I think the ship has sailed on this one for 2.0, but 2.1 seems like a reasonable place to re-raise the issue.

comment:14 Changed 10 years ago by percious

I vote for keeping DBSession the way it is.

comment:15 Changed 10 years ago by Chris Arndt

  • Owner changed from Chris Arndt to mramm

comment:16 Changed 10 years ago by percious

  • Status changed from new to closed
  • Resolution set to wontfix

Im closing this in favor of keeping the api the same. DBSession _is_ a class after all.

comment:17 Changed 9 years ago by percious

  • Milestone changed from 2.1 to 2.1rc1
Note: See TracTickets for help on using tickets.