search

adamrehn / ue4cli

Command-line interface for Unreal Engine 4
https://docs.adamrehn.com/ue4cli/
MIT License
249 stars 45 forks source link

API uses "bare" exceptions in multiple places #67

Open sleeptightAnsiC opened 3 months ago

sleeptightAnsiC commented 3 months ago

Hi @adamrehn

Per our conversation with @TBBle under https://github.com/adamrehn/ue4cli/pull/65, looks like there are several bare exceptions in current API. This is considered a bad practice and non-Pythonic way of catching exceptions - reference1, reference2 It can potentially suppress unwanted exceptions and hide bugs. Something that we don't really want.

Fixing it right now is a bit risky, especially at this stage of the project. It would require some retesting and probably adding more code for edge cases. However, it would be a good change after all. I can try fixing it.

Let me know what do you think!

Example: https://github.com/adamrehn/ue4cli/blob/fed71c1af4cffe3fed9358b08430068ad9454b77/ue4cli/UnrealManagerBase.py#L167-L178

For reference, grep shows at least 7 usages of bare exception:

╰─$ grep -Rno "except:" . 
./ue4cli/UE4BuildInterrogator.py:192:except:
./ue4cli/UnrealManagerBase.py:52:except:
./ue4cli/UnrealManagerBase.py:173:except:
./ue4cli/UnrealManagerBase.py:176:except:
./ue4cli/UnrealManagerWindows.py:9:except:
./ue4cli/UnrealManagerWindows.py:42:except:
./ue4cli/UnrealManagerWindows.py:95:except:
sleeptightAnsiC commented 3 months ago

Similar case appears here. Failure from try-finally block is never handled. https://github.com/adamrehn/ue4cli/blob/fed71c1af4cffe3fed9358b08430068ad9454b77/ue4cli/UE4BuildInterrogator.py#L165-L176

TBBle commented 3 months ago

Similar case appears here. Failure from try-finally block is never handled.

https://github.com/adamrehn/ue4cli/blob/fed71c1af4cffe3fed9358b08430068ad9454b77/ue4cli/UE4BuildInterrogator.py#L165-L176

This one's a bit different, it's just letting any exceptions propagate up (for better or worse but from another function in the same module) but ensuring that clean-up happens either way.

In this case, particularly, someone hitting Control-C into UBT and triggering a KeyboardInterrupt for us should not prevent moving the backup back into place if necessary, but probably doesn't need to be otherwise handled specially here; i.e. this is just an open-coded context manager.