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

python3 migration #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

python3 migration #20

wants to merge 2 commits into from

Conversation

Marwe
Copy link

@Marwe Marwe commented Jul 16, 2020

Applied 2to3 and found a decode to remove, now it works here with python3
hopefully closes: #19
please review and look for other potential migration issues

@Marwe Marwe changed the title pyhton3 migration python3 migration Jul 16, 2020
Copy link
Contributor

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

thanks for the PR!

some stuff that 2to3 has done seems a bit superfluous though, see my comments.
some other stuff seems even wrong.

the .decode() stuff might be valid, but needs checking for py2 compat.

from __future__ import print_function

Copy link
Contributor

Choose a reason for hiding this comment

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

does it still work on py2 with that?

Comment on lines 18 to 22
try:
from ConfigParser import SafeConfigParser as ConfigParser, Error as ConfigParserError
from configparser import SafeConfigParser as ConfigParser, Error as ConfigParserError
except ImportError:
# Python 3
from configparser import ConfigParser, Error as ConfigParserError
Copy link
Contributor

Choose a reason for hiding this comment

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

that doesn't look correct.

the upper block is meant for python 2, so it should be ConfigParser.

if that import fails (e.g. because we are on py3), it is retried in the lower except block.

LIFETIME_MAPPING = dict(zip(LIFETIME_CHOICES, LIFETIME_NAMES))
LIFETIME_MAPPING = dict(list(zip(LIFETIME_CHOICES, LIFETIME_NAMES)))
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain why this is needed?

for k, v in CONFIG_DEFAULTS.items():
for k, v in list(CONFIG_DEFAULTS.items()):
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

for option in CONFIG_DEFAULTS.keys():
for option in list(CONFIG_DEFAULTS.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

for k, v in response.json().items():
for k, v in list(response.json().items()):
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

ftype = mime.from_buffer(first_chunk).decode()
ftype = mime.from_buffer(first_chunk)
Copy link
Contributor

Choose a reason for hiding this comment

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

does it still work on py2 with that?

@ThomasWaldmann
Copy link
Contributor

@Marwe did you see my feedback?

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.

cli not python3 ready?
2 participants