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

Made it django 1.6 and Python 3 compatible #281

Closed
wants to merge 14 commits into from

Conversation

marky1991
Copy link
Contributor

With this patch, the app's tests all pass in django 1.6 and python 3.3. The only remaining failures are query-counting related tests (which I believe are a result of django-tagging being weird. See https://groups.google.com/forum/#!topic/django-blog-zinnia/LRUzSxLlHTM ), two tagging-related tests (I believe this is django-tagging's fault), and tests that import custom_url_shortener.py and custom_spam_checker.py . (Both of which do nothing but raise exceptions, guaranteeing the tests will fail.) The remaining failures fail when using the most recent version of 1.5, under python 2, and using the your most recent version of django-blog-zinnia, so they are not new regressions. I think they could most easily be fixed by replacing django-tagging with a better-supported tagging app. I'd like to address that and the related failures with a later patch.

Some key changes:
-Changes to how views/mixins/archives.py 's get_previous_next_published works.
-Replacement of explicit unicode calls with six.text_type calls.
-Changed how pearson_score works completely to match the algorithm found on the internet. The original version returned values that disagreed with wolfram's calculator. My updated version matches the online calculator's values.
-in preview.py, we now return str(self.preview) instead of the soup itself. (Returning a non-string in str in python 3 raises an exception)
-assert(Not)Equals calls converted to assert(Not)Equal to silence deprecation warnings
-a new urlEqual function was added in tests/utils.py . This is intended to compare urls that have querystrings, where ordering doesn't matter. (So two urls with querystrings in different order should still compare equal)

Some remaining questions:
-CategoryAdminForm in admin/forms.py may or may not be right. The code used originally is not a part of the API so it's not documented at all. (It's not even documented in the source...) The solution currently provided makes the code run and appears to work. (It's the best guess of me and some people on the django-users IRC channel)
-Why does the pearson_score function in comparison.py return 1-r instead of just r? (See the code for context)
-test_vector_builder in tests/comparison.py badly needs to be looked at. I've made it disregard ordering now. I'm not sure if this is desired or not. Right now, the code assumes a stable iteration order over dictionary keys, which is not a valid assumption. I've done my best to fix it, but I don't really know if it matches what you intended or not. If the function wants to preserve order, it needs to be completely rewritten.
-The changes to xmlrpc with respect to bytes vs. strings needs to be looked at. I think I did it correctly, but I'm not 100% sure. (Also tests/utils.py, TestTransport)

…moved the six dependency I had added in earlier. (It's built in to django)
…6 to zinnia.__init__. Corrected the previously-incorrect urlEqual function.
… previous corrections were bad.) Finally have gotten rid of all errors except for the querycount-related failures.
@marky1991
Copy link
Contributor Author

Hmm. I see the build failure. Looking at it now.

@marky1991
Copy link
Contributor Author

Okay. I tried to quickly hack it in, but clearly it's not going to work tonight. I'm going to work on setting up the buildout thing properly on my computer so I can replicate the issues the build runners are having so I can resolve them properly. Will fix some time tomorrow hopefully. Sorry about this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.34%) when pulling 37c3da9 on marky1991:master into a91efe2 on Fantomas42:master.

@marky1991
Copy link
Contributor Author

Phew, finally done!

Edit: Coverage is now reporting that coverage went down as a result of the change. As far as I can tell, the tests already included, if tested using 1.6, would suffice to cover the lines mentioned (the uncovered lines that were added as a result of my commits, that is). I'm not sure what to do about this. If something needs to be done, let me know what it is and I'll do my best to do it.

…ed. Everything should be clean and in order now.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.67%) when pulling ffbfca2 on marky1991:master into a91efe2 on Fantomas42:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bff7b34 on marky1991:master into a91efe2 on Fantomas42:master.

@marky1991
Copy link
Contributor Author

Bump?

@ianare
Copy link

ianare commented Oct 11, 2013

FYI I've merged this in my fork in order to get it to work in Django 1.6 / python 2.7.3 and it works fine.

@marky1991
Copy link
Contributor Author

Issue 296 doesn't appear to be a result of my patch. Still, I'll try to look at it when I get a chance.

@Fantomas42
Copy link
Owner

Hi @marky1991,
I have started my second review of your pull request, and first of all, thank you for your work, and the invested time for doing it. This PR is really huge, not only by the number of affected lines, but also by his quality. I think you have done one of the most important contribution in the Zinnia's codebase.

I will not automatically merge the pull request, because I have some details to review, like removing the compatibility support for Django 1.5. Here the plan to integrate your changes:

  • Release Zinnia v0.13 on pypi
  • Start the development of the v0.14 which will be centered on your PR
  • Modify the Travis build config to handle Python 3 and Django 1.6
  • Start the integration of your PR into a branch
  • Merge the branch into the develop branch when ready
  • Release Zinnia v0.14

What do you think of this ?

Regards

@marky1991
Copy link
Contributor Author

"...like removing the compatibility support for Django 1.5."

My patch doesn't remove support for 1.5. (If this isn't what you meant, ignore this) The same tests pass (or fail) in (python 2.6, django 1.5) and (py3.3, django 1.6).

That plan sounds fine. I was just checking in. : )

@Fantomas42
Copy link
Owner

Yes I see that you have ensured the compatibility with Django 1.5, but I don't want it.
It always lead to future headaches for a few benefits on a project of this size.

@marky1991
Copy link
Contributor Author

Oh, okay. I'll remove the compatability bits then. That'll make the code simpler anyway.

@Fantomas42
Copy link
Owner

As you wish, but I can do it myself during the integration process.
It's not a problem for me.

PS: Good job on the Pearson's score, I think you have pointed out and fixed an historical issue.

@ghost ghost assigned Fantomas42 Dec 3, 2013
@Fantomas42
Copy link
Owner

The tests are now fixed for Django 1.6 with Python 2.
The build on Python 3 does not pass because of django-tagging which is not compatible with Django 1.6.

https://travis-ci.org/Fantomas42/django-blog-zinnia/builds/15096851

A replacement for django-tagging is now mandatory...

@Fantomas42
Copy link
Owner

With this version of django-tagging, https://github.com/Fantomas42/django-tagging,
the builds on Python 3 pass successfully.

@bikeshedder
Copy link

django-tagging is dead. django-taggit (code, documentation) is a better replacement and currently sets the standard for tagging in Django applications:

@Fantomas42
Copy link
Owner

@bikeshedder I need to test more, but a first sight django-taggit does not do the job for me.
And if I need to install extensions over taggit for having the tag cloud, this is definitely not a good replacement.

I'm thinking about another solution, but this problematic needs to be developed in another topic.

@Fantomas42
Copy link
Owner

Pull-request merged in develop @ 49fa3c9

Thank @marky1991

Another time: really good job.

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

Successfully merging this pull request may close these issues.

None yet

5 participants