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.

Sign up for GitHub

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

Jump to bottom

Branweb/python3 upgrade #15

Merged
merged 10 commits into from Mar 31, 2020
Merged

Branweb/python3 upgrade #15

merged 10 commits into from Mar 31, 2020

Conversation

branweb1
Copy link
Contributor

Most changes boil down to:

  • msgpack decodes to python3 bytes rather than unicode strings, so updated the application and tests to reflect that
  • tornado moved away from decorators for async to async/await, so updated the app and the tests to reflect that
  • readme and examples folder contained stale info dating from when this thing used json, not msgpack, so got rid of all that.

branweb1
branweb1 commented Mar 30, 2020
View reviewed changes
.TestDockerfile Outdated
@@ -14,4 +13,4 @@ ADD dev-requirements.txt /app/
RUN pip install -r /app/dev-requirements.txt
ADD carto_renderer /app/carto_renderer

RUN PYTHONPATH=/app py.test -vv /app/carto_renderer
RUN PYTHONPATH=/app py.test -vv -W ignore::DeprecationWarning /app/carto_renderer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could remove this if it's a really bad idea. An test dependency was using a deprecated feature. The dep was updated a few days ago but not released yet: pytest-dev/pytest-asyncio#141

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend waiting for the dep to update.

Copy link
Contributor Author

@branweb1 branweb1 Mar 30, 2020 •
edited

Choose a reason for hiding this comment

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

KAIDA!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

hi Kaida!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi!!!

@branweb1
Copy link
Contributor Author

ah seems like the jenkins build needs fixing too. lame...ok working on it

@chitang
Copy link
Contributor

chitang commented Mar 30, 2020

lgtm, but I am not really a python programmer. Not sure if you want to get a 2nd reviewer.

@branweb1
Copy link
Contributor Author

retest this please

@branweb1
Copy link
Contributor Author

Ok removed "ignore warning" flag from the test-runner command. Going ahead with merge since I'd like this to back in staging for a while. Created this ticket to track updating pytest-asyncio to get rid of the test output warning: https://socrata.atlassian.net/browse/EN-39192

@branweb1 branweb1 merged commit 928dee6 into master Mar 31, 2020
@branweb1 branweb1 deleted the branweb/python3-upgrade branch March 31, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rozap rozap rozap left review comments

@kaidamasaki kaidamasaki kaidamasaki left review comments

Assignees

@chitang chitang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@branweb1 @chitang @rozap @kaidamasaki