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

Remove some remnants of Python2 #243

Merged
merged 3 commits into from
Jul 31, 2024
Merged

Remove some remnants of Python2 #243

merged 3 commits into from
Jul 31, 2024

Conversation

Lorak-mmk
Copy link

@Lorak-mmk Lorak-mmk commented Jul 19, 2023

We recently dropped support for Python < 3.6, but there were still a lot of places in the codebase with workarounds required for those versions - most notably usage of six package.
This PR removes all usages of six, removes six from dependencies, removes all workarounds for older versions that I found and fixes comments / docs references.
There are 2 commits. First one drops six - it is pretty big, but the changes are mostly very simple and mechanical.
Second one removes other mentions / workaround, unrelated to six.

except ImportError:
# OrderedDict from Python 2.7+

# Copyright (c) 2009 Raymond Hettinger
Copy link

Choose a reason for hiding this comment

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

Hey I know this guy... Met him in person when he was in Israel.

@fruch
Copy link

fruch commented Jul 19, 2023

The only concern I see here, is that the next pull from upstream is gonna be painful...

Beside that if all tests pass

Also why do this cleanup now ? and not wait for upstream to do it ?

@Lorak-mmk
Copy link
Author

The only concern I see here, is that the next pull from upstream is gonna be painful...

Beside that if all tests pass

Also why do this cleanup now ? and not wait for upstream to do it ?

I did open this PR in upstream too: datastax#1172
If they accept it, then all is well. If they don't - yes, next pull may be more difficult, but I wouldn't worry about that too much: there isn't that much activity in the upstream, and the resulting conflicts should be easy to resolve.

There isn't any burning reason to do the cleanup now - it just irks me to see all this unnecessary baggage so I want to get rid of it.

@fruch
Copy link

fruch commented Sep 5, 2023

looks like upstream are gonna take it slowly...

maybe we should just merge it, if we confident enough with the changes, maybe it move upstream a bit faster knowing most of it was tested and released by us, merging from upstream next time would be painful

@Lorak-mmk
Copy link
Author

Actually I started to split upstream PR into multiple simple commits, so it's easier to review - maybe that'll speed things up?

@fruch
Copy link

fruch commented Sep 6, 2023

Actually I started to split upstream PR into multiple simple commits, so it's easier to review - maybe that'll speed things up?

I'm o.k. with both options.

@tchaikov
Copy link

just a remark, once six is dropped in the driver, we don't need to have it in https://github.com/scylladb/scylla-cqlsh/blob/3332078f4a66ded92dd7144940aef93ca6edbb7a/bin/cqlsh.py#L119

@fruch
Copy link

fruch commented May 30, 2024

@Lorak-mmk

upstream merged:
datastax#1172

so we should be start a sync with upstream, and close this one

@Lorak-mmk Lorak-mmk added the enhancement New feature or request label Jun 18, 2024
As we no longer support Python 2, there is no reason to
keep this dependency. This commit removes all usages
of six and removes it from dependencies.
There are some stale mentions in docs / comments about
Python versions that are no longer supported. There
are also some workarounds to make driver work with those
versions. This commit removes all mentions and workarounds
that I was able to find.
six.iterkeys() returns an iterator, but Python's
dict.keys() does not, so to pass it to iter() it needs
to be first passed trough iter().
Copy link
Collaborator

@dkropachev dkropachev 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.

Copy link

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@fruch fruch merged commit fab07e1 into scylladb:master Jul 31, 2024
19 checks passed
@Lorak-mmk Lorak-mmk self-assigned this Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants