transientskp / tkp

A transients-discovery pipeline for astronomical image-based surveys
http://docs.transientskp.org/
BSD 2-Clause "Simplified" License
19 stars 15 forks source link

Review SQLalchemy / SQL columns naming #508

Closed timstaley closed 2 months ago

timstaley commented 8 years ago

Before the next major release we should quickly review how our SQLAlchemy models match up to the SQL column names. Currently, the Rejection model class has a .rejectreason_id attribute which refers to the SQL col rejectreason, and a .rejectreason attribute which is used as the SQLAlchemy relationship mapping to the Rejectreason model.

Confusing, but easily fixed by renaming the SQL column to _rejectreasonid to match the model attribute. We should check for similar clashes that could be fixed by a simple rename.

gijzelaerr commented 8 years ago

if I remember correctly I did this intentional. What is actually the problem?

timstaley commented 8 years ago

No particular problem - it's just extremely confusing! I can see why you did it, presumably to match the existing column naming scheme. But we might as well align the attribute names / column names in the next major release. It will take minimal effort.

gijzelaerr commented 8 years ago

so you want to rename all SQL columns from <foreign table name> <foreign table name>_id ?

Before SQLAlchemy I didn't like that since it would make many columns 3 characters longer, but maybe it makes sense now.

timstaley commented 8 years ago

Yep, basically :) I agree there's no point normally, but I think it makes sense when you're using SQLAlchemy.

AntoniaR commented 2 months ago

This issue will likely be resolved in the new redesign of the source association and other SQL commands in TraP R7. Closing this issue.