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.

Sign up for GitHub

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

Jump to bottom

Safe qt importer #804

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Safe qt importer #804

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb0790a
Merge pull request #2 from shotgunsoftware/master
fdan Feb 10, 2021
bf65d7b
safely get qt modules
fdan Feb 11, 2021
7fb2b03
added a contructor check to ensure qt modules memeber is an empty dic…
fdan Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions python/tank/util/qt_importer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,40 +50,42 @@ def __init__(self, interface_version_requested=QT4):
self._qt_version_tuple,
) = self._import_modules(interface_version_requested)

self._modules = self._modules or {}

@property
def QtCore(self):
"""
:returns: QtCore module, if available.
"""
return self._modules["QtCore"] if self._modules else None
return self._modules.get("QtCore")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous syntax implies self._modules may not be set when called so you may need this instead:

Suggested change
return self._modules.get("QtCore")
return self._modules.get("QtCore") if self._modules else None

This covers QtCore or the module not being present in the dict and the dict itself not being set.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the _modules dict is initialised to an empty dict, get should still be safe, i.e.

>>> d = {}
>>> print(d.get('foobar'))
None

If the issue is covering a scenario where _modules is not an attr of the object at all, I don't think we'd need to do that as it's in the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the suggestion by @halil3d above, instead of initializing self._modules in the constructor to an empty dictionary, because for a couple of minor reasons:

  1. If _modules is None, this clearly indicates that the import failed for the requested Qt interface. An empty dict may suggest a different error during the import.
  2. The modules property returns the the value of _modules - so we would be changing the return value from None to {} if the import failed. Which means if callers are checking specifically for None, this would break their code.


@property
def QtGui(self):
"""
:returns: QtGui module, if available.
"""
return self._modules["QtGui"] if self._modules else None
return self._modules.get("QtGui")

@property
def QtWebKit(self):
"""
:returns: QtWebKit module, if available.
"""
return self._modules["QtWebKit"] if self._modules else None
return self._modules.get("QtWebKit")

@property
def QtNetwork(self):
"""
:returns: QtNetwork module, if available.
"""
return self._modules["QtNetwork"] if self._modules else None
return self._modules.get("QtNetwork")

@property
def QtWebEngineWidgets(self):
"""
:returns: QtWebEngineWidgets module, if available.
"""
return self._modules["QtWebEngineWidgets"] if self._modules else None
return self._modules.get("QtWebEngineWidgets")

@property
def binding(self):
Expand Down