Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API uses "bare" exceptions in multiple places #67

Open
sleeptightAnsiC opened this issue Mar 23, 2024 · 2 comments
Open

API uses "bare" exceptions in multiple places #67

sleeptightAnsiC opened this issue Mar 23, 2024 · 2 comments

Comments

@sleeptightAnsiC
Copy link
Contributor

sleeptightAnsiC commented Mar 23, 2024

Hi @adamrehn

Per our conversation with @TBBle under #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:

def getDescriptor(self, dir):
"""
Detects the descriptor file for either an Unreal project or an Unreal plugin in the specified directory
"""
try:
return self.getProjectDescriptor(dir)
except:
try:
return self.getPluginDescriptor(dir)
except:
raise UnrealManagerException('could not detect an Unreal project or plugin in the directory "{}"'.format(dir))

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
Copy link
Contributor Author

sleeptightAnsiC commented Mar 23, 2024

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

# Invoke UnrealBuildTool in JSON export mode (make sure we specify gathering mode, since this is a prerequisite of JSON export)
# (Ensure we always perform sentinel file cleanup even when errors occur)
try:
args = ['-Mode=JsonExport', '-OutputFile=' +jsonFile ] if (self.engineVersion['MajorVersion'] >= 5 or self.engineVersion['MinorVersion'] >= 22) else ['-gather', '-jsonexport=' + jsonFile, '-SkipBuild']
if self.engineVersion['MajorVersion'] >= 5:
self.runUBTFunc('UnrealEditor', platformIdentifier, configuration, args)
else:
self.runUBTFunc('UE4Editor', platformIdentifier, configuration, args)
finally:
if renameSentinel == True:
shutil.move(sentinelBackup, sentinelFile)

@TBBle
Copy link
Contributor

TBBle commented Mar 24, 2024

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

# Invoke UnrealBuildTool in JSON export mode (make sure we specify gathering mode, since this is a prerequisite of JSON export)
# (Ensure we always perform sentinel file cleanup even when errors occur)
try:
args = ['-Mode=JsonExport', '-OutputFile=' +jsonFile ] if (self.engineVersion['MajorVersion'] >= 5 or self.engineVersion['MinorVersion'] >= 22) else ['-gather', '-jsonexport=' + jsonFile, '-SkipBuild']
if self.engineVersion['MajorVersion'] >= 5:
self.runUBTFunc('UnrealEditor', platformIdentifier, configuration, args)
else:
self.runUBTFunc('UE4Editor', platformIdentifier, configuration, args)
finally:
if renameSentinel == True:
shutil.move(sentinelBackup, sentinelFile)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants