SG-29997 Removing Python 2 code #345

eduardoChaucaGallegos · 2024-06-04T17:28:38Z

Removing the use of six package
Useful functions for shotgun.py copied to sgsix.py

julien-lang

Good start.

julien-lang · 2024-06-04T21:18:47Z

shotgun_api3/lib/httplib2/__init__.py

-    del __SSLHandshakeError
+# Generate ssl_error_classes
+import ssl as __ssl
+ssl_error_classes = (__ssl.SSLError, __ssl.CertificateError)


Do we need to keep this variable or can we just replace the code where ssl_error_classes is used by __ssl.SSLError, __ssl.CertificateError?

julien-lang · 2024-06-04T21:19:52Z

shotgun_api3/lib/httplib2/__init__.py

-    # get the python2 fork of httplib2
-    from . import python2 as __httplib2_compat
+# get the python3 fork of httplib2
+from . import python3 as __httplib2_compat


why do we have a "python3" folder in there? Can we simplify how we bundle httplib2 in this repo?

julien-lang · 2024-06-04T21:21:07Z

shotgun_api3/shotgun.py


+from .lib.httplib2 import Http, ProxyInfo, socks, ssl_error_classes
+from .lib.sgtimezone import SgTimezone
+from .lib import sgsix


Can/Should we get rid of that? (or rename the module to "something_utils.py"?)

julien-lang · 2024-06-04T21:24:32Z

shotgun_api3/shotgun.py

@@ -2136,7 +2126,7 @@ def schema_field_update(self, entity_type, field_name, properties, project_entit
            "field_name": field_name,
            "properties": [
                {"property_name": k, "value": v}
-                for k, v in six.iteritems((properties or {}))
+                for k, v in (properties.items() if isinstance(properties, dict)  else {})


I don't think this is correct. What you want is:

Suggested change

for k, v in (properties.items() if isinstance(properties, dict) else {})

for k, v in ((properties if isinstance(properties, dict) else {}).items())

But I think we'd better have this check done earlier in the function instead.

julien-lang · 2024-06-05T14:46:27Z

shotgun_api3/lib/httplib2/__init__.py

I don't think we want to edit httplib2. This is third-party we bundle. But can we update to a newer version if we drop py2?

Actually, it was manually updated by #202 so we might want to clean up (or simply update to a newer version)

julien-lang

Make sure to look #202 when addressing this PR

julien-lang

I am starting to get scared by this PR. This is super big and going all over the place. We might want to chunk it.

julien-lang · 2024-06-06T16:36:31Z

HISTORY.rst

+===================
+- Drop support for Python 2.x.x
+- Drop use of six package
+- Documentation update


We only change this file when we prepare a release of python-api. Not when we work on the code.

julien-lang · 2024-06-06T16:37:14Z

setup.py



 setup(
    name='shotgun_api3',
-    version='3.6.0',
+    version='3.7.0',


No, not in this PR

julien-lang · 2024-06-06T16:38:31Z

shotgun_api3/lib/httplib2/__init__.py

Actually, it was manually updated by #202 so we might want to clean up (or simply update to a newer version)

julien-lang · 2024-06-06T16:41:21Z

shotgun_api3/shotgun.py

@@ -3575,7 +3546,7 @@ def _make_call(self, verb, path, body, headers):
            attempt += 1
            try:
                return self._http_request(verb, path, body, req_headers)
-            except ssl_error_classes as e:
+            except (SSLError, CertificateError) as e:
                # Test whether the exception is due to the fact that this is an older version of
                # Python that cannot validate certificates encrypted with SHA-2. If it is, then


This comment talks about Python 2.5 and below. We might want to clean that up. I started that in #347.

carlos-villavicencio-adsk

Looks good so far. It would be great to depend less from six or sgsix functions.

I also opened a few questions.

carlos-villavicencio-adsk · 2024-06-06T16:47:09Z

shotgun_api3/lib/README.md

@@ -23,17 +23,11 @@ See bugs:

 The version of `mimetypes` bundled should be updated manually if necessary, however it is unlikely this will be needed, as it is only used for Python versions 2.7.0 - 2.7.9, and newer Python versions simply use the native `mimetypes` module.

-### six


I don't see six removed in this PR. We should keep this until it is completely removed.

carlos-villavicencio-adsk · 2024-06-06T16:50:00Z

shotgun_api3/lib/sgutils.py

Why is the reason of this rename? There are components importing sgsix and this change could potentially break them. Example: https://github.com/shotgunsoftware/tk-core/blob/master/python/tank/commands/core_localize.py#L26

@eduardoChaucaGallegos Crazy idea: can we leave these methods on six.py for now and keep a mini-version of six.py only with the methods implemented in this file?

My intention is to avoid renaming the package and impact the TK components that depends on these functions.

We can discuss this internally.

Not so crazy, I kind of like that.

I'm going to revert changes in this MR to only apply steps for first phase (that mini-version of six.py can be sgutils.py )

that mini-version of six.py can be sgutils.py

Yes, but keeping the same name of six.py

That way, dependant component won't need to rename the import for ensure_*

carlos-villavicencio-adsk · 2024-06-06T16:57:46Z

shotgun_api3/shotgun.py

+                if isinstance(k, str):
+                    k = sgutils.ensure_str(k)
+                if isinstance(v, str):
+                    v = sgutils.ensure_str(v)


I think this was needed when we have different string types in Python 2 (string and unicode). I guess ensure_str is not needed anymore making these two conditionals useless. This is just an assumption.

carlos-villavicencio-adsk · 2024-06-06T16:58:06Z

shotgun_api3/shotgun.py

-            if isinstance(value, six.string_types):
-                return six.ensure_text(value)
+            if isinstance(value, str):
+                return sgutils.ensure_text(value)


Same as my previous comment.

carlos-villavicencio-adsk · 2024-06-06T16:58:46Z

shotgun_api3/shotgun.py

@@ -4253,7 +4224,7 @@ def _send_form(self, url, params):
                else:
                    raise ShotgunError("Unanticipated error occurred %s" % (e))

-            return six.ensure_text(result)
+            return sgutils.ensure_text(result)


Same here, I think we can just use

Suggested change

return sgutils.ensure_text(result)

return str(result)

carlos-villavicencio-adsk · 2024-06-06T17:00:23Z

shotgun_api3/shotgun.py

+            value = sgutils.ensure_text(value)
+            key = sgutils.ensure_text(key)


same here, I think we can safely replace str casting method isntead.

eduardoChaucaGallegos added 2 commits June 4, 2024 12:23

six package is not used anymore, still testing

a1243d0

vscode filde deleted

21a0ab1

julien-lang reviewed Jun 4, 2024

View reviewed changes

julien-lang reviewed Jun 5, 2024

View reviewed changes

eduardoChaucaGallegos added 5 commits June 6, 2024 00:35

using htlib for python3 and fixing references for new sgutils.py

2e32ef0

updating setup and HISTORY file

8e0b7c4

deleting more code related to python2

5f7dd2e

tests fixed

e28c56f

print deleted and also a test file

717af51

eduardoChaucaGallegos marked this pull request as ready for review June 6, 2024 16:21

eduardoChaucaGallegos requested a review from carlos-villavicencio-adsk June 6, 2024 16:21

julien-lang reviewed Jun 6, 2024

View reviewed changes

carlos-villavicencio-adsk reviewed Jun 6, 2024

View reviewed changes

carlos-villavicencio-adsk mentioned this pull request Jul 4, 2024

SG-29997 first phase python2 dropping shotgunsoftware/tk-core#986

Merged

eduardoChaucaGallegos closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SG-29997 Removing Python 2 code #345

SG-29997 Removing Python 2 code #345

eduardoChaucaGallegos commented Jun 4, 2024

julien-lang left a comment

julien-lang Jun 4, 2024

julien-lang Jun 4, 2024

julien-lang Jun 4, 2024

julien-lang Jun 4, 2024

julien-lang Jun 5, 2024

julien-lang Jun 6, 2024

julien-lang left a comment

julien-lang left a comment

julien-lang Jun 6, 2024

julien-lang Jun 6, 2024

julien-lang Jun 6, 2024

julien-lang Jun 6, 2024

carlos-villavicencio-adsk left a comment

carlos-villavicencio-adsk Jun 6, 2024

carlos-villavicencio-adsk Jun 6, 2024

carlos-villavicencio-adsk Jul 4, 2024

julien-lang Jul 4, 2024

eduardoChaucaGallegos Jul 4, 2024

carlos-villavicencio-adsk Jul 4, 2024

carlos-villavicencio-adsk Jun 6, 2024

carlos-villavicencio-adsk Jun 6, 2024

carlos-villavicencio-adsk Jun 6, 2024

carlos-villavicencio-adsk Jun 6, 2024

	for k, v in (properties.items() if isinstance(properties, dict) else {})
	for k, v in ((properties if isinstance(properties, dict) else {}).items())

		@@ -23,17 +23,11 @@ See bugs:

		The version of `mimetypes` bundled should be updated manually if necessary, however it is unlikely this will be needed, as it is only used for Python versions 2.7.0 - 2.7.9, and newer Python versions simply use the native `mimetypes` module.

		### six

		value = sgutils.ensure_text(value)
		key = sgutils.ensure_text(key)

SG-29997 Removing Python 2 code #345

SG-29997 Removing Python 2 code #345

Conversation

eduardoChaucaGallegos commented Jun 4, 2024

julien-lang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien-lang left a comment

Choose a reason for hiding this comment

julien-lang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-villavicencio-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment