Opened 6 years ago

Closed 2 years ago

#5535 closed task (no-action)

Remove unused local variables

Reported by: simenheg Owned by:
Priority: trivial Milestone: 0.13.0
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.

Attachments (2)

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

Download all attachments as: .zip

Change History (7)

by simenheg, 6 years ago

Attachment: issue_5535.patch added

comment:1 by Boris Bobrov, 6 years ago

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 :)

by simenheg, 6 years ago

Attachment: issue_5535_rev2.patch added

comment:2 by simenheg, 6 years ago

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 by Boris Bobrov, 6 years ago

Owner: simenheg removed
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 :(

comment:4 by Ben Sturmfels, 2 years ago

Milestone: 0.13.0

comment:5 by Ben Sturmfels, 2 years ago

Resolution: no-action
Status: reviewclosed

Hi simenheg,

Thanks very much for the patch, and sorry we didn't get to it at the time. Unfortunately these sort of cleanups can be difficult to merge because there's a reasonable chance something might get broken along the way, so requiring very careful review and testing. Also, the patch has aged a bit now, so no longer merges cleanly on current master.

I'm going to try work through some of these unused variables as well as unused import using pyflakes and try to get them cleaned up in time.

Thanks again. Closing this ticket.

Ben

Note: See TracTickets for help on using tickets.