ue4cli icon indicating copy to clipboard operation
ue4cli copied to clipboard

API uses "bare" exceptions in multiple places

Open sleeptightAnsiC opened this issue 1 year ago • 2 comments

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 avatar Mar 23 '24 15:03 sleeptightAnsiC

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

sleeptightAnsiC avatar Mar 23 '24 18:03 sleeptightAnsiC

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.

TBBle avatar Mar 24 '24 14:03 TBBle