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

SG-15210 Python3 port #73

Merged
merged 10 commits into from
Jul 28, 2020
Merged

SG-15210 Python3 port #73

merged 10 commits into from
Jul 28, 2020

Conversation

eshokrgozar
Copy link

@eshokrgozar eshokrgozar commented Jul 2, 2020

Fixes to make the tool work with Python3.

@eshokrgozar eshokrgozar requested review from jfboismenu and removed request for jfboismenu July 6, 2020 14:12
@eshokrgozar eshokrgozar requested a review from jfboismenu July 6, 2020 15:18

# Grab the setting's value type.
value_type = type(args[name])

# We assume unicode and str to be equivalent for these checks because
# args come back from Django as unicode but are first set by the
# Registrar as str.
if value_type == unicode:
if value_type.__name__ == "unicode":

Choose a reason for hiding this comment

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

instanceof(value, six.text_type)

is the cleaner way to check for this. When we clean up our code in the long future where Python 2 is not supported anymore finding checks for six.text_type will be super easy.

Copy link
Author

Choose a reason for hiding this comment

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

I thought about doing this. There's an inconsistency in the way the argument type validation is done across the different plugins. Most of them use the type.__name__ like this (look at the values under the type key):

args_to_check = {
"source_frames_field": {"sg_type": "number", "type": "str"},
"target_tc_field": {"sg_type": "timecode", "type": "str"},
"fps": {"type": "float"},
}

value_type = type(args[name]).__name__
# We assume unicode and str to be equivalent for these checks because
# args come back from Django as unicode but are first set by the
# Registrar as str.
if value_type == "unicode":
value_type = "str"

And a couple of them use the type itself:

args_to_check = {
"project_ids": {"type": [list], "allow_empty": False},
"entity_types": {"type": [list], "allow_empty": False},
"plugins_field": {
"type": [str],
"allow_empty": False,
"entity": "TaskTemplate",
"sg_type": ["list", "entity_type"],
"required_values": ["N/A", args["plugins_field_value"]],
},
"plugins_field_value": {"type": [str], "allow_empty": False},
"trash_old_tasks": {"type": [bool], "allow_empty": False},
"overwrite_field_values": {"type": [bool], "allow_empty": False},
"exclude_fields": {"type": [list], "allow_empty": True},
}

I decided to do the string compare to keep it consistent with the other ones. I have no problem changing it to the isinstance method. But I did want to point this out to see if you think we should change all the other ones to not use the string matching either.

Choose a reason for hiding this comment

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

Better is the enemy of good. If this is the pattern used in the repo, let's not rock the boat.

Copy link

@jfboismenu jfboismenu left a comment

Choose a reason for hiding this comment

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

Looks good to me! I only have two suggestions.

  • please add a requirements.txt and update the README.md (or maybe the Wiki?). We now have a dependency on six.
  • please use isinstance as mentioned in the comments.

@eshokrgozar eshokrgozar requested a review from jfboismenu July 9, 2020 14:57
Copy link

@jfboismenu jfboismenu left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@eshokrgozar eshokrgozar merged commit 3a4b4d9 into master Jul 28, 2020
This was referenced Jul 29, 2020
@000paradox000 000paradox000 deleted the SG-15210_Python3_port branch December 7, 2022 22:47
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

Successfully merging this pull request may close these issues.

2 participants