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

Fixes more Python2 and Python3 common codebase #4327

Merged
merged 9 commits into from
Apr 12, 2019
Merged

Fixes more Python2 and Python3 common codebase #4327

merged 9 commits into from
Apr 12, 2019

Conversation

kushaldas
Copy link
Contributor

Status

Work in progress

Description of Changes

Fixes a few more failed tests.

Changes proposed in this pull request:

Testing

make test

make test-python3

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

@kushaldas kushaldas changed the title Morepyyy3 Fixes more Python2 and Python3 common codebase Apr 9, 2019
@kushaldas kushaldas marked this pull request as ready for review April 10, 2019 10:43
heartsucker
heartsucker reviewed Apr 10, 2019
View reviewed changes
Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

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

I left some notes on improvements that mostly center around readability. Nothing here is wrong, but I think if we're going to be using six everywhere, we might want to simplify it a bit.

securedrop/crypto_util.py Outdated Show resolved Hide resolved
securedrop/secure_tempfile.py Outdated Show resolved Hide resolved
securedrop/secure_tempfile.py Show resolved Hide resolved
securedrop/tests/test_i18n_tool.py Outdated Show resolved Hide resolved
securedrop/tests/test_i18n_tool.py Outdated Show resolved Hide resolved
securedrop/tests/test_integration.py Outdated Show resolved Hide resolved
securedrop/tests/test_journalist.py Show resolved Hide resolved
redshiftzero
redshiftzero approved these changes Apr 12, 2019
View reviewed changes
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

looks great! thanks @kushaldas (I implemented a couple of @heartsucker's suggestions in commits appended to the end) - this is good to merge when the staging job passes ✨

@redshiftzero redshiftzero merged commit c8e7ec9 into pyyy3 Apr 12, 2019
@redshiftzero redshiftzero deleted the morepyyy3 branch April 12, 2019 01:52
@redshiftzero redshiftzero mentioned this pull request Apr 16, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heartsucker heartsucker heartsucker left review comments

@redshiftzero redshiftzero redshiftzero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kushaldas @heartsucker @redshiftzero