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

Opened 11 years ago

Last modified 10 years ago

Improvement of patch #1290

Reported by: chrisz Owned by: anonymous
Priority: high Milestone: 1.0.2
Component: SQLAlchemy Version: 1.0
Severity: trivial Keywords: sqlalchemy identity tables
Cc:

Description

This improves the patch in ticket #1290. Instead of calling class_mapper, we can simply use the mapper attribute. Also use docstrings instead of comments. The patch again applies to the 1.0 branch and the trunk.

Attachments

satables2.patch Download (2.2 KB) - added by chrisz 11 years ago.
Improvement of patch #1290
1198.patch Download (1.1 KB) - added by kuffs 10 years ago.
chrisz's patch, updated for current trunk
1292.patch Download (2.0 KB) - added by kuffs 10 years ago.
chrisz's patch, updated for current trunk

Change History

Changed 11 years ago by chrisz

Improvement of patch #1290

comment:1 Changed 11 years ago by jorge.vargas

#1251 is a generalization of this

comment:2 Changed 11 years ago by chrisz

Did you mean a different patch? #1251 is an unrelated issue actually.

comment:3 Changed 11 years ago by jorge.vargas

  • Owner set to anonymous
  • Priority changed from low to normal
  • Component changed from unassigned to SQLAlchemy
  • Severity changed from minor to trivial

ahh sorry I got mix up, all the mappers of SA are confusing :(

if someone that is using SA wants to apply this please go ahead.

comment:4 Changed 11 years ago by fredlin

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

fixed in [2582], thanks!

comment:5 Changed 11 years ago by plewis

  • Status changed from closed to reopened
  • Resolution fixed deleted

I don't think this patch got applied cleanly. saprovider still has references to class_mapper, which raise a NameError?

comment:6 Changed 11 years ago by chrisz

  • Priority changed from normal to high

Yes, the class_mapper() call is still there in both files. It must be replaced like in the attached patch. I have set this to high because SA support is now broken.

comment:7 Changed 11 years ago by fredlin

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

fix 'again' in [2590]:( thanks :-P

comment:8 Changed 11 years ago by chrisz

  • Status changed from closed to reopened
  • Resolution fixed deleted

But now it is the same as it was before. The idea was to completely get rid of importing and using class_mapper() because it is unnecessary complicated since you can get the mapper with the mapper attribute. I.e. instead of

class_mapper(xyz_class).local_table.create(checkfirst=True) 

you would write

xyz_class.mapper.local_table.create(checkfirst=True) 

Changed 10 years ago by kuffs

chrisz's patch, updated for current trunk

Changed 10 years ago by kuffs

chrisz's patch, updated for current trunk

comment:9 Changed 10 years ago by kuffs

Please disregard 1198.patch, I chose the wrong file.

comment:10 Changed 10 years ago by fredlin

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

fixed in [2802], thanks kuffs

comment:11 Changed 10 years ago by fredlin

sorry I miss the last committer to chrisz,

Anyway, thanks both of you, chrisz and kuffs :-)

(I'll update the CHANGLOG in next commit)

Note: See TracTickets for help on using tickets.