Opened 4 years ago

Closed 5 weeks 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.

Subtickets

Attachments (2)

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

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by simenheg

Attachment: issue_5535.patch added

comment:1 Changed 4 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 4 years ago by simenheg

Attachment: issue_5535_rev2.patch added

comment:2 Changed 4 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 4 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 :(

comment:4 Changed 5 weeks ago by Ben Sturmfels

Milestone: 0.13.0

comment:5 Changed 5 weeks ago by Ben Sturmfels

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.