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 #1251 (closed enhancement: fixed)

Opened 12 years ago

Last modified 12 years ago

Enhancement proposal for SQLAlchemy default model

Reported by: chrisz Owned by: anonymous
Priority: low Milestone: 1.0.2
Component: SQLAlchemy Version: 1.0
Severity: normal Keywords: assign_mapper
Cc:

Description

In the SQLAlchemy default model, the mapping is done with assign_mapper instead of the usual mapper. This is a good idea since it simplifies accessing the objects later (which should be exploited in saprovider.py by the way). However, instead of repeatedly writing

assign_mapper(session.context, ...)

in the model, I suggest adding the following lines

def mapper(*args, **kw):
    """Use assign_mapper with our context."""
    return assign_mapper(session.context, *args, **kw)

and then simply writing

mapper(session.context, ...)

This has also the advantage that you can use the standard mapper instead of assign_mapper (which is in sqlalchemy.ext, not in the core sqlalchemy) by just removing the lines above.

Attachments

assign_mapper.patch Download (2.2 KB) - added by chrisz 12 years ago.
Patch for the default model
assign.patch Download (2.1 KB) - added by chrisz 12 years ago.
Modified patch, using "assign" instead of "mapper"

Change History

Changed 12 years ago by chrisz

Patch for the default model

comment:1 Changed 12 years ago by chrisz

  • Type changed from defect to enhancement

comment:2 Changed 12 years ago by jorge.vargas

see also #1292

comment:3 Changed 12 years ago by chrisz

Instead of

mapper(session.context, ...)

I meant

mapper(...)

of course. I.e. in Python 2.5 notation:

mapper = functools.partial(assign_mapper, session.context)

comment:4 Changed 12 years ago by volvox

Having a bound session for domain classes and objects is no slight change in semantics..

I don't really think overriding a function name that is already used in sqlalchemy would be a good thing. This change is not backward compatible, and could cause confusion when somebody will ask for help in the mailing list (more confusions for those that do _not_ ask, of course).

comment:5 Changed 12 years ago by chrisz

If you think that's a problem, then we can also call the function something different, e.g. simply "assign". The main point is to avoid the unnecessary code and make it easy to switch beween mapper and assign_mapper. Using a verb for that function is also more intuitive, since what matters here is the assignment, not the fact that we get a mapper object back which is discarded anyway.

comment:6 Changed 12 years ago by volvox

Yes, assign() makes sense.

Changed 12 years ago by chrisz

Modified patch, using "assign" instead of "mapper"

comment:7 Changed 12 years ago by fredlin

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

accepted in [2797], thanks!

Note: See TracTickets for help on using tickets.