chore: remove Python 2 compatibility code - #651
Open
hdd wants to merge 1 commit into
Open
Conversation
Remove all Python 2/3 compatibility shims as the codebase requires Python 3.13: - Remove `six` dependency and imports - Replace `six.string_types` with `str` - Remove `six.PY2` conditional branches - Replace `dict.iteritems()` with `dict.items()` - Replace `unicode` type checks with `str`/`bytes` - Simplify base64 encoding/decoding (no PY2 path needed) - Replace `type(x) == str` with `isinstance(x, str)` The codebase now uses only Python 3 idioms throughout. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR modernizes the codebase to remove Python 2 compatibility layers via ChangesPython 3 Modernization Across Modules
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage report
This PR does not seem to contain any modification to coverable code. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/framework-core/source/ftrack_framework_core/asset/asset_info.py (1)
124-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace
eval()in_check_asset_info_dependencies
dict(eval(dependency))inlibs/framework-core/source/ftrack_framework_core/asset/asset_info.pycan execute arbitrary code if any dependency string is attacker-controlled. Within this repo,_check_asset_info_dependencieshas no other call sites, andcreate()only buildsconstants.asset.DEPENDENCY_IDSfromasset_version_entity["uses_versions"][...]["id"]values (not stringified dicts), but the unsafeevalstill needs removal.🛡️ Safer parsing
+import ast ... - if isinstance(dependency, str): - dict_dept = dict(eval(dependency)) + if isinstance(dependency, str): + dict_dept = dict(ast.literal_eval(dependency))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/framework-core/source/ftrack_framework_core/asset/asset_info.py` around lines 124 - 129, The code in _check_asset_info_dependencies uses eval(dependency) which is unsafe; replace it with a safe parser such as ast.literal_eval (import ast) or json.loads depending on the expected input, e.g. parse the dependency string into a dict via ast.literal_eval and then pass that dict to FtrackAssetInfo (retain the existing variables dict_dept and dep_asset_info). Also add minimal exception handling around the parse to catch ValueError/SyntaxError (log/raise as appropriate) so malformed strings don't silently crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/framework-core/source/ftrack_framework_core/asset/asset_info.py`:
- Around line 63-65: The decode_options path logs an error when
asset_info_options is empty but then continues and causes json.JSONDecodeError;
update the method (the block handling asset_info_options in decode_options) to
return early with a safe default (e.g., {} or None) immediately after logging
the error so it doesn't call base64.b64decode/json.loads on an empty string;
ensure the change references the asset_info_options variable and uses
self.logger.error in the same method to preserve the existing log behavior.
In `@libs/framework-core/source/ftrack_framework_core/client/__init__.py`:
- Line 385: The loop over options.get("tool_configs") can raise TypeError when
tool_configs is None; update the iterator in the block that contains the line
'for tool_config_name in options.get("tool_configs")' to guard against None
(e.g., use options.get("tool_configs", []) or check and set tool_configs =
options.get("tool_configs") or [] before the loop) so the code safely handles
missing or null tool_configs values.
- Line 478: The condition uses dialog_options.get("docked") but dialog_options
can be None; update the check to guard for None before calling .get(), for
example change the conditional around the existing logic to ensure
dialog_options is truthy (e.g. if dialog_options and
dialog_options.get("docked") and dock_func) or use a safe fallback (e.g. if
(dialog_options or {}).get("docked") and dock_func) so dialog_options.get is
never invoked on None; modify the conditional that currently reads `if
dialog_options.get("docked") and dock_func:` accordingly.
- Around line 426-435: The validation incorrectly uses isinstance(dialog_class,
FrameworkDialog) which rejects valid dialog classes; update the check in
Client.run_dialog (the dialog_class validation block) to verify that
dialog_class is a class and a subclass of FrameworkDialog (e.g., use
inspect.isclass(dialog_class) and issubclass(dialog_class, FrameworkDialog)) and
log/raise the same error if that test fails; ensure you import inspect if needed
and preserve the existing error_message, logger.error and raising behavior.
---
Outside diff comments:
In `@libs/framework-core/source/ftrack_framework_core/asset/asset_info.py`:
- Around line 124-129: The code in _check_asset_info_dependencies uses
eval(dependency) which is unsafe; replace it with a safe parser such as
ast.literal_eval (import ast) or json.loads depending on the expected input,
e.g. parse the dependency string into a dict via ast.literal_eval and then pass
that dict to FtrackAssetInfo (retain the existing variables dict_dept and
dep_asset_info). Also add minimal exception handling around the parse to catch
ValueError/SyntaxError (log/raise as appropriate) so malformed strings don't
silently crash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a71b87d-5109-43cd-a140-c8450e2e92f9
📒 Files selected for processing (4)
libs/framework-core/source/ftrack_framework_core/asset/asset_info.pylibs/framework-core/source/ftrack_framework_core/client/__init__.pylibs/utils/source/ftrack_utils/string/__init__.pyprojects/nuke-studio/source/ftrack_nuke_studio/exception.py
Comment on lines
63
to
+65
| if not asset_info_options: | ||
| self.logger.error("asset_info_options is empty") | ||
| if six.PY2: | ||
| return json.loads(base64.b64decode(asset_info_options)) | ||
| else: | ||
| return json.loads( | ||
| base64.b64decode(asset_info_options).decode('ascii') | ||
| ) | ||
| return json.loads(base64.b64decode(asset_info_options).decode("ascii")) |
There was a problem hiding this comment.
decode_options logs an error on empty input but then crashes anyway.
When asset_info_options is empty, the error is logged and execution continues into base64.b64decode("") → "" → json.loads(""), which raises json.JSONDecodeError. The log message is misleading because the call still fails with an unrelated exception. Return early (or a safe default) after logging.
🛠️ Proposed fix
if not asset_info_options:
self.logger.error("asset_info_options is empty")
+ return None
return json.loads(base64.b64decode(asset_info_options).decode("ascii"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not asset_info_options: | |
| self.logger.error("asset_info_options is empty") | |
| if six.PY2: | |
| return json.loads(base64.b64decode(asset_info_options)) | |
| else: | |
| return json.loads( | |
| base64.b64decode(asset_info_options).decode('ascii') | |
| ) | |
| return json.loads(base64.b64decode(asset_info_options).decode("ascii")) | |
| if not asset_info_options: | |
| self.logger.error("asset_info_options is empty") | |
| return None | |
| return json.loads(base64.b64decode(asset_info_options).decode("ascii")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/framework-core/source/ftrack_framework_core/asset/asset_info.py` around
lines 63 - 65, The decode_options path logs an error when asset_info_options is
empty but then continues and causes json.JSONDecodeError; update the method (the
block handling asset_info_options in decode_options) to return early with a safe
default (e.g., {} or None) immediately after logging the error so it doesn't
call base64.b64decode/json.loads on an empty string; ensure the change
references the asset_info_options variable and uses self.logger.error in the
same method to preserve the existing log behavior.
| for config in sublist | ||
| ] | ||
| for tool_config_name in options.get('tool_configs'): | ||
| for tool_config_name in options.get("tool_configs"): |
There was a problem hiding this comment.
Handle missing tool_configs before iterating.
At Line 385, options.get("tool_configs") can be None, which causes a TypeError when iterated.
Suggested fix
- for tool_config_name in options.get("tool_configs"):
+ for tool_config_name in options.get("tool_configs", []):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for tool_config_name in options.get("tool_configs"): | |
| for tool_config_name in options.get("tool_configs", []): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/framework-core/source/ftrack_framework_core/client/__init__.py` at line
385, The loop over options.get("tool_configs") can raise TypeError when
tool_configs is None; update the iterator in the block that contains the line
'for tool_config_name in options.get("tool_configs")' to guard against None
(e.g., use options.get("tool_configs", []) or check and set tool_configs =
options.get("tool_configs") or [] before the loop) so the code safely handles
missing or null tool_configs values.
Comment on lines
426
to
435
| if dialog_class: | ||
| if not isinstance(dialog_class, FrameworkDialog): | ||
| error_message = ( | ||
| 'The provided class {} is not instance of the base framework ' | ||
| 'widget. Please provide a supported widget.'.format( | ||
| "The provided class {} is not instance of the base framework " | ||
| "widget. Please provide a supported widget.".format( | ||
| dialog_class | ||
| ) | ||
| ) | ||
| self.logger.error(error_message) | ||
| raise Exception(error_message) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how run_dialog is called and whether dialog_class is passed as a class.
rg -nP --type=py -C2 '\brun_dialog\s*\('
rg -nP --type=py -C2 'dialog_class\s*='Repository: ftrackhq/integrations
Length of output: 3311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate FrameworkDialog definition
rg -n --type=py "class\s+FrameworkDialog|FrameworkDialog\s*=" libs/framework-core/source/ftrack_framework_core
# Inspect run_dialog implementation around the validation and usage of dialog_class
sed -n '390,520p' libs/framework-core/source/ftrack_framework_core/client/__init__.py
# Find how registry.dialogs stores dialog entries and specifically what "extension" is
rg -n --type=py "registry\.dialogs|dialogs\s*=" libs/framework-core/source/ftrack_framework_core/client/__init__.py libs/framework-core/source/ftrack_framework_core -S
rg -n --type=py "\"extension\"\s*:" libs/framework-core/source/ftrack_framework_core/client/__init__.py libs/framework-core/source/ftrack_framework_core -S
sed -n '520,700p' libs/framework-core/source/ftrack_framework_core/client/__init__.pyRepository: ftrackhq/integrations
Length of output: 8269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect registry.add() and how dialogs are stored
sed -n '1,240p' libs/framework-core/source/ftrack_framework_core/registry/__init__.py
rg -n --type=py "def add\(" libs/framework-core/source/ftrack_framework_core/registry/__init__.py
rg -n --type=py "dialogs\s*=" libs/framework-core/source/ftrack_framework_core/registry/__init__.py
rg -n --type=py "extension_type=\"dialog\"|extension\"?\s*:\s*extension" libs/framework-core/source/ftrack_framework_core/registry/__init__.py
sed -n '240,420p' libs/framework-core/source/ftrack_framework_core/registry/__init__.pyRepository: ftrackhq/integrations
Length of output: 7402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=py "run_dialog\s*\(.*dialog_class\s*=" .
rg -n --type=py "dialog_class\s*=" libs/framework-core/source/ftrack_framework_core
rg -n --type=py "add\(\s*extension_type\s*=\s*\"dialog\"|extension_type\s*=\s*'dialog'" -S .Repository: ftrackhq/integrations
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any call sites or usage of dialog_class across the repo
rg -n --type=py "\bdialog_class\b" .
rg -n --type=py "run_dialog\s*\(" libs/framework-core/source/ftrack_framework_core tests tools . || trueRepository: ftrackhq/integrations
Length of output: 2508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect cookiecutter template call site
sed -n '1,120p' "tools/cookiecutter-framework-project/{{cookiecutter.project_name}}/source/{{ cookiecutter.package_name }}/__init__.py"
# Inspect standalone UI manual test call site
sed -n '1,140p' tests/framework/manual/standalone_ui_test.pyRepository: ftrackhq/integrations
Length of output: 5920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where dialogs are added to the registry
rg -n --type=py "extension_type\s*=\s*['\"]dialog['\"]" .
rg -n --type=py "\badd\(\s*extension_type\s*=\s*['\"]dialog['\"]" .
# Find dialog classes exported/declared as framework widgets
rg -n --type=py "class\s+\w*Dialog\b" libs/framework-core/source/ftrack_framework_core/widget/dialog.py libs/framework-core/source/ftrack_framework_core/widget .
rg -n --type=py "FrameworkDialog" . -SRepository: ftrackhq/integrations
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect FrameworkDialog base class details
sed -n '1,220p' libs/framework-core/source/ftrack_framework_core/widget/dialog.py
# Find any concrete dialog implementations inheriting FrameworkDialog
rg -n --type=py "class\s+\w*Dialog\w*\(.*FrameworkDialog" libs/framework-core/source/ftrack_framework_core || true
rg -n --type=py "FrameworkDialog" libs/framework-core/source/ftrack_framework_core/widget || true
# Search for any extensions that declare dialog widgets (likely in a 'widget' or 'dialog' module)
rg -n --type=py "extension_type\s*=\s*['\"]dialog['\"]" . || true
rg -n --type=py "extension_type\s*:\s*['\"]dialog['\"]" . || true
rg -n --type=py "\"dialog\"" libs/framework-core/source/ftrack_framework_core -S || trueRepository: ftrackhq/integrations
Length of output: 7390
Fix dialog_class validation to accept dialog widget subclasses
Client.run_dialog() later instantiates dialog_class via dialog = dialog_class(...) and stores/compares it as the registry "extension". The current isinstance(dialog_class, FrameworkDialog) check rejects valid dialog classes.
File: libs/framework-core/source/ftrack_framework_core/client/__init__.py
Lines: 426-435
if dialog_class:
if not isinstance(dialog_class, FrameworkDialog):
error_message = (
"The provided class {} is not instance of the base framework "
"widget. Please provide a supported widget.".format(
dialog_class
)
)
self.logger.error(error_message)
raise Exception(error_message)Suggested fix
- if not isinstance(dialog_class, FrameworkDialog):
+ if not (
+ isinstance(dialog_class, type)
+ and issubclass(dialog_class, FrameworkDialog)
+ ):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/framework-core/source/ftrack_framework_core/client/__init__.py` around
lines 426 - 435, The validation incorrectly uses isinstance(dialog_class,
FrameworkDialog) which rejects valid dialog classes; update the check in
Client.run_dialog (the dialog_class validation block) to verify that
dialog_class is a class and a subclass of FrameworkDialog (e.g., use
inspect.isclass(dialog_class) and issubclass(dialog_class, FrameworkDialog)) and
log/raise the same error if that test fails; ensure you import inspect if needed
and preserve the existing error_message, logger.error and raising behavior.
| self.dialog = dialog | ||
| # If a docking function is provided, use it | ||
| if dialog_options.get('docked') and dock_func: | ||
| if dialog_options.get("docked") and dock_func: |
There was a problem hiding this comment.
Guard dialog_options before calling .get().
At Line 478, dialog_options defaults to None, so dialog_options.get(...) can raise AttributeError.
Suggested fix
- if dialog_options.get("docked") and dock_func:
+ if dialog_options and dialog_options.get("docked") and dock_func:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if dialog_options.get("docked") and dock_func: | |
| if dialog_options and dialog_options.get("docked") and dock_func: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/framework-core/source/ftrack_framework_core/client/__init__.py` at line
478, The condition uses dialog_options.get("docked") but dialog_options can be
None; update the check to guard for None before calling .get(), for example
change the conditional around the existing logic to ensure dialog_options is
truthy (e.g. if dialog_options and dialog_options.get("docked") and dock_func)
or use a safe fallback (e.g. if (dialog_options or {}).get("docked") and
dock_func) so dialog_options.get is never invoked on None; modify the
conditional that currently reads `if dialog_options.get("docked") and
dock_func:` accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cleanup from remaning py2 compatibility code paths and imports
Summary by CodeRabbit
Refactor
Chores
Note: These changes are internal improvements with no impact on user-facing functionality.