Uh oh!
There was an error while loading. Please reload this page.
(cleanup) remove Python 2 remaining items - #727
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the Python 2 cleanup by removing remaining unicode/UnicodeMixin compatibility shims and updating tests/docs/code paths to assume Python 3-only semantics (the project now requires Python >=3.9).
Changes:
- Removes Python 2-era unicode patterns (
u'',__unicode__,UnicodeMixin) and normalizes string handling across driver and cqlengine. - Simplifies Python-version conditionals/fallback imports (e.g.,
WeakSetimports) and applies formatting-only refactors in several modules/tests. - Updates unit/integration tests and Sphinx config to reflect Python 3-only behavior and representations.
Reviewed changes
Copilot reviewed 34 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_types.py | Replaces u'' literals with plain str in type read/write tests. |
| tests/unit/test_row_factories.py | Removes Python 3.0–3.6 conditional expectations for namedtuple creation. |
| tests/unit/test_orderedmap.py | Updates unicode-key tests to Python 3 str keys. |
| tests/unit/test_metadata.py | Replaces u'' literals with str in metadata CQL export tests. |
| tests/unit/test_marshalling.py | Updates UTF8/unicode expectations to Python 3 str and cleans up ordered map inserts. |
| tests/unit/advanced/test_insights.py | Removes Python-version-specific namespace logic and reformats expected dicts. |
| tests/integration/standard/test_query.py | Updates unicode query strings/column names to Python 3 str. |
| tests/integration/standard/test_cluster.py | Updates expected row tuples to Python 3 str. |
| tests/integration/cqlengine/model/test_udts.py | Updates unicode literals to Python 3 str. |
| tests/integration/cqlengine/model/test_model_io.py | Updates unicode literals to Python 3 str in model IO assertions. |
| tests/integration/cqlengine/model/test_class_construction.py | Mostly formatting + Python 3 iterator usage (next(iter(...))) and string literal normalization. |
| tests/integration/cqlengine/columns/test_validation.py | Removes old Python-version branches and normalizes string usage/formatting in validation tests. |
| setup.py | Removes Python 2-era subprocess gating and refactors extension/doc build setup logic. |
| docs/conf.py | Normalizes string literals and formatting in Sphinx configuration. |
| cassandra/query.py | Python 3 string/formatting cleanup; keeps namedtuple fallback paths but modernizes literals/layout. |
| cassandra/pool.py | Removes legacy WeakSet fallback and reformats/shard-aware related code blocks. |
| cassandra/io/asyncorereactor.py | Removes legacy WeakSet fallback and modernizes literals/formatting. |
| cassandra/encoder.py | Deprecates Python 2 “unicode” semantics and standardizes encoding/quoting behavior for Python 3. |
| cassandra/datastax/graph/query.py | Python 3 string/formatting cleanup and minor readability refactors. |
| cassandra/datastax/graph/graphson.py | Python 3 cleanup + formatting; updates docs/comments describing supported Python types. |
| cassandra/datastax/graph/fluent/_query.py | Python 3 string/formatting cleanup and improves readability of traversal query generation. |
| cassandra/cqlengine/statements.py | Removes UnicodeMixin usage and converts __unicode__ implementations to __str__. |
| cassandra/cqlengine/operators.py | Removes UnicodeMixin usage and converts operator stringification to __str__. |
| cassandra/cqlengine/named.py | Converts __unicode__ to __str__ and normalizes string literals. |
| cassandra/cqlengine/models.py | Removes UnicodeMixin usage and normalizes string literals/formatting across model machinery. |
| cassandra/cqlengine/functions.py | Removes UnicodeMixin usage and converts __unicode__ to __str__. |
| cassandra/cqlengine/init.py | Removes UnicodeMixin definition entirely. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mykaul
commented
Mar 5, 2026
Fixed all comments. |
mykaul
commented
Mar 5, 2026
@copilot code review[agent] - please re-review |
mykaul
commented
Mar 16, 2026
Review: Rebased onto master, Python 3 expert reviewRebase StatusRebased all 18 commits onto current Review FindingsThe 18 commits are semantically correct. The bulk of the diff is formatting (quote normalization, import reformatting). All major semantic changes verified:
Issues Found
Finding #1 is the most important — it's a latent runtime error that would crash if an unknown prepared statement is executed (error logging path). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 38 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
4778136 to
42e3d08CompareImportant Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
mykaul
commented
Jul 29, 2026
Rebased onto current Rebase: two real conflicts, both from unrelated master churn — Fixed 3 issues introduced by the rebase/rewrite itself (pyflakes before/after diff on every touched file caught these):
Addressed the "Issues Found" from the earlier review comment on this PR:
Verified, not fixed (checked against current master, no action needed):
Full |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
tests/unit/test_row_factories.py:1
- The test asserts no warnings unconditionally, but the docstring explicitly says the bug was fixed in Python 3.7+. If your supported matrix still includes Python < 3.7, this will fail on those runtimes. Consider either (a) skipping/branching the assertion based on
sys.version_info, or (b) updating the test to enforcewarnings.simplefilter("always")inside the context and asserting the expected warning only for affected versions; also re-add an assertion that field access works (e.g.,rows[0].col0) to keep the behavioral coverage from the prior test.
tests/unit/test_orderedmap.py:1 - These assertions became tautological after removing
u''literals, so the test no longer validates the normalization behavior described by PYTHON-231. To preserve intent under Python 3, use two distinct but equivalent key representations that the type layer normalizes (e.g., bytes vs str keys if supported by the serializer) and assert they resolve to the same stored entry.
tests/unit/test_orderedmap.py:1 - These assertions became tautological after removing
u''literals, so the test no longer validates the normalization behavior described by PYTHON-231. To preserve intent under Python 3, use two distinct but equivalent key representations that the type layer normalizes (e.g., bytes vs str keys if supported by the serializer) and assert they resolve to the same stored entry.
tests/integration/standard/test_metadata.py:1122 - The Python 2-only UDT export integration test (
test_export_keyspace_schema_udts) was removed entirely, which drops coverage for schema export of UDTs. Consider porting it to Python 3 by making the comparison deterministic (e.g., avoid relying on dict iteration order by parsing/normalizing the exported CQL, or comparing against multiple expected fragments in an order-insensitive way) so the behavior remains covered without requiring Python 2.7.
@greaterthancass21
def test_case_sensitivity(self):
"""
cassandra/cluster.py:1611
- The bare
except:will swallow unexpected failures (e.g., runtime errors insideeventletreactor,TypeErrorifself.connection_classis not a class), making debugging harder and potentially masking real regressions. Prefer catching the expected exceptions explicitly (typicallyImportError, and optionallyTypeErrorforissubclass) and letting other exceptions propagate.
try:
from cassandra.io.eventletreactor import EventletConnection
is_eventlet = issubclass(self.connection_class, EventletConnection)
except:
# Eventlet is not available or can't be detected
return tpe_class(**kwargs)
cassandra/encoder.py:110
- The Sphinx
deprecateddirective normally requires a version argument (.. deprecated:: X.Y). As written, doc builds may emit warnings or fail depending on your Sphinx settings. Provide the deprecation version (or switch to a narrative note if you don’t want to formally deprecate the API).
Encodes a string value with quote escaping.
.. deprecated::
This method is unused internally since Python 2 support was
removed (``str`` is always unicode on Python 3). It is kept
for backward compatibility with user subclasses of
:class:`Encoder`.
setup.py:111
- The function name
get_subdrinameappears to be a typo, which makes it harder to discover/understand. If it’s part of a public/internal API used elsewhere, consider introducing a correctly named wrapper (e.g.,get_subdirname) and keeping the old name as a backward-compatible alias with a deprecation path.
def get_subdriname(directory_path):
try:
# List only subdirectories in the given directory
subdirectories = [name for name in os.listdir(directory_path)
if os.path.isdir(os.path.join(directory_path, name))]
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
tests/unit/test_orderedmap.py:1
- These assertions became tautological after removing
u''literals and no longer validate the intended normalization behavior (PYTHON-231). To keep the test meaningful on Python 3, consider asserting equivalence across different input types that serialize identically for the key type (e.g.,strkeys vsbyteskeys for UTF8 serialization), so the test still exercises the normalization path.
tests/unit/test_orderedmap.py:1 - These assertions became tautological after removing
u''literals and no longer validate the intended normalization behavior (PYTHON-231). To keep the test meaningful on Python 3, consider asserting equivalence across different input types that serialize identically for the key type (e.g.,strkeys vsbyteskeys for UTF8 serialization), so the test still exercises the normalization path.
tests/unit/test_row_factories.py:1 - This test now unconditionally asserts that no warnings are emitted for >255 columns. Previously it handled Python <3.7 where
collections.namedtuplehad the 255-fields limitation (PYTHON-893). If the supported runtime matrix still includes Python 3.6 (or older), this will fail. Consider restoring a version guard/skip (or asserting the fallback behavior) unless the project minimum Python version is 3.7+.
tests/unit/test_row_factories.py:1 - This test now unconditionally asserts that no warnings are emitted for >255 columns. Previously it handled Python <3.7 where
collections.namedtuplehad the 255-fields limitation (PYTHON-893). If the supported runtime matrix still includes Python 3.6 (or older), this will fail. Consider restoring a version guard/skip (or asserting the fallback behavior) unless the project minimum Python version is 3.7+.
cassandra/util.py:232 - The custom
WeakSetimplementation was removed fromcassandra.util. Ifcassandra.util.WeakSetwas part of the library's de-facto public surface (even if undocumented), this is a breaking change for downstream imports. Consider re-exportingweakref.WeakSetunder the same name (possibly with a deprecation period) to preserve backward compatibility while still removing the Python 2 fallback implementation.
class SortedSet(object):
'''
cassandra/cqlengine/init.py:27
- Removing
UnicodeMixincan be a breaking change for external users who may import it or subclass cqlengine components expecting its__str__behavior. If this project maintains compatibility guarantees, consider keeping a minimal deprecatedUnicodeMixinalias (or migration shim) for at least one release cycle.
class ValidationError(CQLEngineException):
pass
cassandra/encoder.py:110
- Sphinx's
deprecateddirective typically requires a version argument (e.g.,.. deprecated:: 3.x). As written,.. deprecated::without a version may fail doc builds or render incorrectly. Consider adding the appropriate version (or replacing with plain text if you don't want it treated as a formal deprecation directive).
Encodes a string value with quote escaping.
.. deprecated::
This method is unused internally since Python 2 support was
removed (``str`` is always unicode on Python 3). It is kept
for backward compatibility with user subclasses of
:class:`Encoder`.
cassandra/cqltypes.py:507
- This changes
AsciiType.serializebehavior to explicitly acceptbytes(and to alwaysencode('ascii')forstr). Please add/adjust unit tests to cover bothbytesandstrinputs (including non-ASCIIstrraising as expected), since this is a functional behavior change from prior Python 3 behavior.
if isinstance(var, bytes):
return var
return var.encode('ascii')
cassandra/cqltypes.py:784
- Similarly,
UTF8Type.serializenow explicitly acceptsbytesand encodesstr. Adding focused unit coverage here (bytes passthrough, str encoding, and common edge cases) would help prevent regressions in marshalling/encoding behavior.
if isinstance(ustr, bytes):
return ustr
return ustr.encode('utf-8')
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
setup.py (1)
80-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve subprocess failure causes.
Raise both
RuntimeErrorinstances withfrom excso traceback tooling retains the originalCalledProcessErroras the direct cause.Proposed fix
- raise RuntimeError("Documentation step '%s' failed: %s: %s" % ("build_ext", exc, exc.output))+ raise RuntimeError("Documentation step '%s' failed: %s: %s" % ("build_ext", exc, exc.output)) from exc ... - raise RuntimeError("Documentation step '%s' failed: %s: %s" % (mode, exc, exc.output))+ raise RuntimeError("Documentation step '%s' failed: %s: %s" % (mode, exc, exc.output)) from excAlso applies to: 89-90
🤖 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 `@setup.py` around lines 80 - 81, Update both RuntimeError raises in the subprocess failure handlers to explicitly chain the original CalledProcessError using from exc, preserving each exception as the direct traceback cause while keeping the existing error messages unchanged.Source: Linters/SAST tools
🤖 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 `@cassandra/cluster.py`:
- Around line 1606-1624: Update the exception handler around the
EventletConnection import and issubclass check in the executor-selection flow to
catch only ImportError, and explicitly chain the re-raise when propagating it.
Preserve the fallback for Eventlet being unavailable, while allowing
KeyboardInterrupt, SystemExit, and unrelated import or detection failures to
surface instead of silently returning the default executor.
In `@cassandra/datastax/graph/graphson.py`:
- Around line 61-65: The type mapping table contains two conflicting date
entries; update the duplicate date rows so their graph-mode applicability is
explicitly distinguished, following the existing duration annotation, or
consolidate them into one unambiguous mapping while preserving the correct
Python type for each mode.
---
Nitpick comments:
In `@setup.py`:
- Around line 80-81: Update both RuntimeError raises in the subprocess failure
handlers to explicitly chain the original CalledProcessError using from exc,
preserving each exception as the direct traceback cause while keeping the
existing error messages unchanged.
🪄 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: 49211c83-fb63-4e7c-a4d3-2d3a83eaef7e
📒 Files selected for processing (36)
cassandra/cluster.pycassandra/connection.pycassandra/cqlengine/__init__.pycassandra/cqlengine/functions.pycassandra/cqlengine/models.pycassandra/cqlengine/named.pycassandra/cqlengine/operators.pycassandra/cqlengine/query.pycassandra/cqlengine/statements.pycassandra/cqltypes.pycassandra/datastax/graph/fluent/_query.pycassandra/datastax/graph/graphson.pycassandra/datastax/graph/query.pycassandra/encoder.pycassandra/io/asyncorereactor.pycassandra/metadata.pycassandra/pool.pycassandra/protocol.pycassandra/query.pycassandra/util.pysetup.pytests/integration/cqlengine/columns/test_validation.pytests/integration/cqlengine/model/test_class_construction.pytests/integration/cqlengine/model/test_model_io.pytests/integration/cqlengine/model/test_udts.pytests/integration/cqlengine/query/test_queryset.pytests/integration/standard/test_cluster.pytests/integration/standard/test_metadata.pytests/integration/standard/test_query.pytests/integration/standard/test_types.pytests/unit/advanced/test_insights.pytests/unit/test_marshalling.pytests/unit/test_metadata.pytests/unit/test_orderedmap.pytests/unit/test_row_factories.pytests/unit/test_types.py
💤 Files with no reviewable changes (4)
- cassandra/cqlengine/init.py
- cassandra/connection.py
- tests/integration/cqlengine/query/test_queryset.py
- cassandra/protocol.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
absolute_import is the default behavior in Python 3 and has been since Python 3.0. The import was only needed for Python 2 compatibility.
UnicodeMixin was a Python 2 compatibility shim that made __str__ delegate to __unicode__. In Python 3, __str__ is the native method and UnicodeMixin serves no purpose. Affected classes: QueryValue, BaseQueryOperator, AbstractQueryableColumn, ValueQuoter, BaseClause, BaseCQLStatement.
In Python 3, __str__ is the native string method. The __unicode__ methods were a Python 2 convention used together with UnicodeMixin. Affected files: statements.py, operators.py, named.py, models.py, query.py, functions.py.
__nonzero__ was the Python 2 name for the boolean conversion method. Python 3 uses __bool__ directly.
weakref.WeakSet has been available since Python 2.7 and is always present in Python 3. Remove the try/except fallback imports in cluster.py, pool.py, and asyncorereactor.py, and delete the custom _IterationGuard and WeakSet classes from util.py (~210 lines).
mykaul
commented
Jul 30, 2026
CI note: the only real (non-flaky-NLB) failure here was on Test wheels building / Build wheels for macos-arm on macos-14: (job run) This is a pre-existing, unrelated flaky-test bug in It's already fixed on #766 (
#766 remains the canonical fix for this bug and should still land on its own for traceability/history; this is just a duplicate application to get this PR's CI green in the meantime. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/encoder.py:106
- Sphinx's
deprecateddirective requires a version argument. Because this method is explicitly included bydocs/api/cassandra/encoder.rst, the warning-as-error documentation build will fail while parsing this docstring. Add the release version, consistent with the other deprecation directive incluster.py.
.. deprecated::
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
cassandra/cluster.py:1608
- This import can raise
AttributeErrorwhen an incompatible Eventlet is installed (the module-level Eventlet probe already handles that exact Python 3.12 failure at lines 106–111). Because everyClustercreates its executor through this path, such an installation now prevents even non-Eventlet clusters from being constructed instead of falling back toThreadPoolExecutor. HandleAttributeErrorhere as well (or reuse the safely probed module-levelEventletConnection).
except ImportError:
cassandra/encoder.py:110
- Sphinx's
deprecateddirective requires a version argument. This method is explicitly included bydocs/api/cassandra/encoder.rst, and the docs CI builds with warnings as errors, so the argument-less directive will fail the documentation build. Add the release in which this method is deprecated (or use a non-versioned note if it is not formally deprecated).
.. deprecated::
This method is unused internally since Python 2 support was
removed (``str`` is always unicode on Python 3). It is kept
for backward compatibility with user subclasses of
:class:`Encoder`.
cassandra/datastax/graph/graphson.py:59
- The standard-library address classes are named
IPv4AddressandIPv6Address; the all-capsIPVspelling makes the updated type table inaccurate.
inet | gx:InetAddress | gx:InetAddress | str, IPV4Address/IPV6Address
tests/unit/test_row_factories.py:47
- Use “column lists” rather than “columns lists.”
… 3.9+
- cluster.py: remove 'if sys.version_info >= 3.7' guard around eventlet
check (always true since we require Python 3.9+). Update error message.
- cluster.py: fix latent bug in ResponseFuture's unknown-prepared-statement
error path: bytes.encode('hex') is a Python 2 idiom that raises
AttributeError on Python 3 bytes objects (no encode() method); use
bytes.hex() instead.
- test_insights.py: remove 'if sys.version_info > (3,)' guard around
namespace suffix (always true).
- test_row_factories.py: remove NAMEDTUPLE_CREATION_BUG flag and
Python 3.0-3.6 warning path (bug was fixed in Python 3.7).subprocess has been part of the standard library since Python 2.4. Also fixes: - Use sys.executable instead of hardcoded 'python' for subprocess calls - Use 'python -m sphinx' instead of 'sphinx-build' for docs - Fix get_subdriname() bug: was iterating over characters of a string path instead of calling os.listdir() on the path - Remove duplicate is_macos definition
In Python 3, the u'' prefix on string literals is a no-op (str is always unicode). Remove 150 occurrences across 15 files. Also update cql_encode_unicode() in encoder.py to pass the string directly to cql_quote() instead of encoding to UTF-8 bytes first, since Python 3 strings are always unicode.
- test_validation.py: remove 'if sys.version_info < (3, 1)' blocks that tested unichr() (only available in Python 2). Also fix 'class DataType():' to 'class DataType:'. - test_metadata.py: delete test_export_keyspace_schema_udts which was skipped on all Python versions except 2.7.
The workaround pre-loaded the UTF-8 encoding module to avoid a deadlock when importing from multiple threads. This bug was fixed in Python 3.3.
Also ports the HashableMock thread-safety fix from PR scylladb#766 (fix/hashable-mock-thread-safety) to unblock this PR's own CI: the "Build wheels for macos-arm on macos-14" job was failing on tests/unit/test_host_connection_pool.py::HostConnectionTests::test_successful_wait_for_connection with `TypeError: __hash__ method should return an integer not 'MagicMock'`. This is a pre-existing, unrelated flaky-test bug (not introduced by this Python-2-cleanup PR); PR scylladb#766 is the canonical fix and should still land separately. MagicMixin.__init__ replaces __hash__ on the mock's type with a MagicMock object, which is not thread-safe under concurrent hash() calls (e.g. `connection in self._trash` in pool.py). Fix by restoring a plain, id-based __hash__ function on the class after super().__init__() runs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
cassandra/encoder.py:106
- The Sphinx
deprecateddirective requires a version argument. Because this method is included bydocs/api/cassandra/encoder.rst, the new argument-less directive makes the documentation build fail instead of rendering the deprecation notice.
.. deprecated::
cassandra/cluster.py:1608
- This local import only catches
ImportError, but the module-level import explicitly also catchesAttributeErrorbecause incompatible Eventlet releases raise it on Python 3.12. Since this method runs during everyClusterinitialization, merely having that Eventlet version installed will now break clusters that do not use Eventlet instead of falling back to the standard executor. HandleAttributeErrorhere as well (or reuse the already guarded module-level import).
except ImportError:
tests/unit/test_orderedmap.py:185
- These assertions now compare each lookup with the identical expression, so they can pass even if
OrderedMapSerializedKeystops normalizing equivalent key representations. Usebytesversusstr, which serialize to the same UTF8 key under Python 3, to preserve the normalization regression coverage.
tests/unit/test_types.py:328 - After removing the
uprefix, this assertion is identical to the following line and no longer exercises a distinct case. Keep only one copy.
Pre-review checklist
This is 100% OpenCode's work. So take it with a grain of salt, and I need to go over it. I can also cherry-pick each one separately. I've asked it to separate as much as possible to independent items.
./docs/source/.Fixes:annotations to PR description.