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

Updates for Python 3 #722

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

Updates for Python 3 #722

wants to merge 21 commits into from

Conversation

bradchoate
Copy link
Collaborator

Upgrades code to be Python 3 compatible. Initial port using 2to3, then incremental pass over the code to correct issues discovered with failing unit tests. Also brought all Python requirements up to their latest version. Moved a copy of torndb (no long maintained) into the repository with a few updates for Python 3.

Plan is to release this against a Ubutnu 22.04 LTS host and base Docker image for web/worker instances.

This update also uses the latest nginx server, version 1.25.1.

#554

Upgrades code to be Python 3 compatible. Initial port using 2to3,
then incremental pass over the code to correct issues discovered with
failing unit tests. Also brought all Python requirements up to their
latest version. Moved a copy of torndb (no long maintained) into
the repository with a few updates for Python 3.

Plan is to release this against a Ubutnu 22.04 LTS host and base
Docker image for web/worker instances.

This update also uses the latest nginx server, version 1.25.1.
@@ -127,7 +127,7 @@ <h3>Cool Tools! Cool Tools!</h3>
<time datetime="{{sharedfile.created_at}}" title="{{sharedfile.feed_date()}}">{{sharedfile.pretty_created_at()}}</time>
</span>
</span>
{% if sharedfile.previous_sharedfile_id > 0 %}
{% if sharedfile.previous_sharedfile_id or 0 > 0 %}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Python 2 would evaluate None > 0 as False. But Python 3 throws an exception when comparing None with an int.

@@ -19,7 +19,7 @@
TEST_MODULES = [
'test.AccountTests',
'test.CommentTests',
'test.ExternalAccountTests',
# 'test.ExternalAccountTests',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have Twitter API service now, so we don't need to test it.

@@ -161,7 +161,7 @@ <h4>You have canceled your subscription, but it will remain active
{% end %}

<h4>Your Membership Plan: <a href="/account/membership">{{ plan_name }}</a></h4>
{% if user.stripe_plan_id == "mltshp-double" and user.stripe_plan_rate > 24 %}
{% if user.stripe_plan_id == "mltshp-double" and user.stripe_plan_rate is not None and user.stripe_plan_rate > 24 %}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another case where None was possibly compared with an int.

@@ -134,6 +136,7 @@ def transcode_sharedfile(sharedfile_id):
animated = True
except EOFError:
pass
im.close()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some explicit close calls on handles to try to resolve some warnings during test execution. It didn't resolve everything, so I need to take a closer look, but there's no harm in doing these to my knowledge.

if twitter_account:
via_twitter_account = " via @{0}".format(twitter_account['screen_name'])
api.update_status('https://mltshp.com/p/%s "%s"%s' % (sf['share_key'], title[:90], via_twitter_account))
# The Twitter API is dead.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RIP Twitter.

@@ -71,7 +71,7 @@ def make_zip_file(for_user=None):
extension = "png"

if extension == "":
print(sf.content_type)
print((sf.content_type))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think these extra parenthesis are needed (they probably don't have any effect, either).

coverage
coveralls
coverage==6.5.0
coveralls==3.3.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a later version of coverage available, but 6.5.0 was the latest supported version for coveralls 3.3.1.

base36_id=base36_id,
sharedfile_id=sharedfile_id,
since_id=since_id,
max_id=max_id,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some leftover debugging code. These can be removed.

@dphiffer
Copy link
Member

Debt jubilee! I got it running locally and the tests all pass. One thing that's just slightly weird with the way make test runs is I see a mixture of progress dots . and HTTP log messages. I don't think it really matters or needs to be fixed, but it is weird all the same.

TIL about the if [...] or 0 > 0 trick you called out above.

@bradchoate
Copy link
Collaborator Author

Yeah, the logging is annoying for the tests and I'm not sure what is causing it. I'll try to disable it, since it's adding unnecessary noise (although it did turn out to be helpful in fixing that final test failure).

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.

3 participants