Opened 2 years ago

Last modified 2 years ago

#5535 review task

Remove unused local variables

Reported by: simenheg Owned by:
Priority: trivial Milestone:
Component: programming Keywords:
Cc: simenheg@… Parent Tickets:

Description

After a similar cleanup in ticket #5534, I discovered a bunch of other local variables that were assigned to but never used.

Subtickets

Attachments (2)

issue_5535.patch (19.6 KB) - added by simenheg 2 years ago.
issue_5535_rev2.patch (19.2 KB) - added by simenheg 2 years ago.

Download all attachments as: .zip

Change History (5)

Changed 2 years ago by simenheg

Attachment: issue_5535.patch added

comment:1 Changed 2 years ago by Boris Bobrov

Status: newaccepted

i have some concerns about 5535. Although the variables are indeed not used, sometimes there is no sense in leaving the function call/class instantiation too. For example, in mediagoblin/db/migrations.py:def federation_graveyard(db) you remove metadata = MetaData(bind=db.bind) and just leave MetaData(...). I don't see any reason to call MetaData() here.

The same for config = pluginapi. ...

Some of these statements should be removed completely. By removing just the variables we actually lose evidence of how bad we are :)

Changed 2 years ago by simenheg

Attachment: issue_5535_rev2.patch added

comment:2 Changed 2 years ago by simenheg

Thanks for your comments, breton. I've attached a new patch where I'm more ruthless about what I rip out.

I'm still not sure about whether Table(...) statements can have side effects (I don't known SQLAlchemy very well), but from the comment at lines 312-313 in test_sql_migrations.py it seems like it. Could that be the case?

comment:3 Changed 2 years ago by Boris Bobrov

Owner: simenheg deleted
Status: acceptedreview

So, i tried to review this patch several times, but i don't understand some of the things that are being or not being removed. What does inspect_table do, for example, and what happens if we don't call it?

Could someone please have a look at the patch and confirm that it is good? Sqlalchemy is magic for me :(

Note: See TracTickets for help on using tickets.