esitarski / CrossMgr

Cyclo Cross Management Application
MIT License
41 stars 21 forks source link

Fix issue opening RaceDB dialog #23

Closed mwalling closed 5 years ago

mwalling commented 5 years ago

Python 3 removes e from scope when leaving the exception handling logic. Catch the exception to a different variable, then assign to e.

Demonstrated in this test program:

e = None

try:
    raise ValueError("Whomp whomp")
except Exception as e:
    pass

print(e)

The PyDoc explains this:

When an exception has been assigned using as target, it is cleared at the end of the except clause. This is as if

except E as N:
    foo

was translated to

except E as N:
    try:
        foo
    finally:
        del N

This means the exception must be assigned to a different name to be able to refer to it after the except clause. Exceptions are cleared because with the traceback attached to them, they form a reference cycle with the stack frame, keeping all locals in that frame alive until the next garbage collection occurs.

I also learned this today, and have a bunch of my own code to clean up now!

mwalling commented 5 years ago

Before fix:

2019-06-06T17:55:50 (v3.0.11 Windows) start: CrossMgr 3.0.11
2019-06-06T17:55:50 (v3.0.11 Windows) lang: "en"
2019-06-06T17:55:50 (v3.0.11 Windows) page: Actions
2019-06-06T17:55:50 (v3.0.11 Windows) page: Keypad
2019-06-06T17:55:51 (v3.0.11 Windows) page: GeneralInfoProperties
2019-06-06T17:56:00 (v3.0.11 Windows) call: menuOpenRaceDB(<<MainWin>>, <<CommandEvent>>)
Traceback (most recent call last):
  File "CrossMgr\Utils.py", line 608, in new_f
  File "CrossMgr\MainWin.py", line 3049, in menuOpenRaceDB
  File "CrossMgr\RaceDB.py", line 159, in __init__
  File "CrossMgr\RaceDB.py", line 255, in refresh
UnboundLocalError: local variable 'e' referenced before assignment
2019-06-06T17:56:02 (v3.0.11 Windows) call: onCloseWindow(<<MainWin>>, <<CloseEvent>>)
esitarski commented 5 years ago

Thanks for pointing that out. I have made the fix to RaceDB.py which is available in the latest update. I scanned the rest of the code and it seems to be OK.

I would argue that this violates Python design as it is inconsistent with the "Principle of Least Astonishment". Using a variable in any other block does not "del" it from scope if it is used beforehand. This applies to the usual "for" and "if" blocks, but also "with" blocks using the "as" syntax too. Why are "try" blocks different?

e = None

for i in range(10): e = i else: e = False

print(e) # OK

try: with open('somefile.txt') as e: pass

except: pass

print(e) # OK

def f(): print(e) # OK

print(e) # OK

try: raise ValueError("Whomp whomp") except Exception as e: pass print(e) # WOT?

On Thu, Jun 6, 2019 at 6:34 PM Mark Walling notifications@github.com wrote:

2019-06-06T17:55:50 (v3.0.11 Windows) lang: "en" 2019-06-06T17:55:50 (v3.0.11 Windows) page: Actions 2019-06-06T17:55:50 (v3.0.11 Windows) page: Keypad 2019-06-06T17:55:51 (v3.0.11 Windows) page: GeneralInfoProperties 2019-06-06T17:56:00 (v3.0.11 Windows) call: menuOpenRaceDB(<>, <>) Traceback (most recent call last): File "CrossMgr\Utils.py", line 608, in new_f File "CrossMgr\MainWin.py", line 3049, in menuOpenRaceDB File "CrossMgr\RaceDB.py", line 159, in init File "CrossMgr\RaceDB.py", line 255, in refresh UnboundLocalError: local variable 'e' referenced before assignment 2019-06-06T17:56:02 (v3.0.11 Windows) call: onCloseWindow(<>, <>)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/esitarski/CrossMgr/pull/23?email_source=notifications&email_token=AABGXKKLLYZJERTYOMOJL5LPZGGIZA5CNFSM4HVLIU3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXELKQI#issuecomment-499692865, or mute the thread https://github.com/notifications/unsubscribe-auth/AABGXKNML5IP3M52RO5DBHLPZGGIZANCNFSM4HVLIU3A .

--

Edward Sitarski

mwalling commented 5 years ago

Reading the docs, it looks like a performance improvement:

Exceptions are cleared because ... all locals in that frame alive until the next garbage collection occurs.

Still annoying. The fact that variables move out of inner scopes I like, coming from my day job as a Java developer.

Oh well, at least the reason the RaceDB dialog wasn't opening is fixed.