Skip to content
This repository has been archived by the owner on Jan 16, 2023. It is now read-only.

Python 2/3 Support #22

Merged
merged 7 commits into from
Mar 8, 2014
Merged

Python 2/3 Support #22

merged 7 commits into from
Mar 8, 2014

Conversation

archen
Copy link
Contributor

@archen archen commented Feb 16, 2014

Added some logic for Python 3 support that maintains Python 2 support

@@ -120,7 +132,7 @@ def handle_noargs(self, **options):
clear_display = 'This will overwrite existing files!'

if self.interactive:
confirm = raw_input(u"""
confirm = input(u"""
Copy link
Owner

Choose a reason for hiding this comment

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

This code is copied straight from Django itself so I'd rather maintain it being as similar as possible to Django. I am not sure what the differences between raw_input and input are though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The big difference is raw_input doesn't exist in Python 3.x. Which Django module did you pull this code from? I'd be interested in seeing how they handle this issue for Py2/3 compatibility.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

If you want to update the code so that it reflects the latest stable version of Django, I'd be fine with that. Alternatively, if you'd be able to modify the output in a cleaner way without duplicating all the code, that'd be preferable ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say modify the output, you mean try to find a way to override the output message in the function provided by Django 1.6+'s collectstatic?

It looks like they import 'input' from the included six utility in Django 1.6. If I can't figure out the DRYest way of doing it, I'll just remove the logic up top that looks for the built-in and use the six module instead.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, that's what I meant.

Cool, just make sure it's compatible with 1.4.x as well 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason that you're declaring this function anyway? I don't see anything collectfast specific happening in handle_noargs. Why override at all?

edit: I removed handle_noargs entirely from a local branch and collectstatic still behaves as expected.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I missed that line in the template. I can't think of a better way at this point to override only the template and summary due to the order of execution. For now, I'll wrap testing on Django 1.4+ and Python 2/3 and push the changes for the input call still overriding handle_noargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing done. All is well in my environment with Python 2/3 and Django 1.4 - 1.7 using S3 and boto with collectfast.

long_description was originally commented out due to incompatibility with the Python buildpack in Heroku and Dokku. Uncommented for wide distribution as the compatibility issue isn't the fault of this code, but the pip version (1.3) in the buildpack itself.
… Imports Python 2/3 compatible input from django.utils.six
antonagestam added a commit that referenced this pull request Mar 8, 2014
@antonagestam antonagestam merged commit 3fb9a0f into antonagestam:master Mar 8, 2014
@antonagestam
Copy link
Owner

@archen Thank you for taking your time with this ticket!

@archen
Copy link
Contributor Author

archen commented Mar 15, 2014

@antonagestam absolutely! hopefully someone else finds it as useful as I did. Thanks for putting the original package together!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants