Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Made it django 1.6 and Python 3 compatible #281

Closed
wants to merge 14 commits into from

5 participants

@marky1991

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)

@marky1991

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

@marky1991

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

Coverage Status

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

@marky1991

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.

@coveralls

Coverage Status

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

@coveralls

Coverage Status

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

@marky1991

Bump?

@ianare

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

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
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

"...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
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

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

@Fantomas42
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.

@Fantomas42 Fantomas42 was assigned
@Fantomas42
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
Owner

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

@bikeshedder

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

@Fantomas42
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
Owner

Pull-request merged in develop @ 49fa3c9

Thank @marky1991

Another time: really good job.

@Fantomas42 Fantomas42 closed this
@fcoelho fcoelho referenced this pull request from a commit in semcomp/semcomp-next
@fcoelho fcoelho ainda não tá pronto pra testar em python 3.3
o django-blog-zinnia tem como depedência django-tagging, que ainda não
é compatível com python 3:

Fantomas42/django-blog-zinnia#281 (comment)
09d464f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 12, 2013
  1. @marky1991
  2. @marky1991
  3. @marky1991

    Small bugfix.

    marky1991 authored
Commits on Sep 13, 2013
  1. @marky1991

    Made the ping-related tests pass. (Down to ~70 failures now). Also re…

    marky1991 authored
    …moved the six dependency I had added in earlier. (It's built in to django)
Commits on Sep 16, 2013
  1. @marky1991

    Corrected the date failures, converted ranges to lists in the paginat…

    marky1991 authored
    …ion tests, and added a urlEqual function
Commits on Sep 19, 2013
  1. @marky1991

    Made templatetags date-related failures pass. Also moved is_before_1_…

    marky1991 authored
    …6 to zinnia.__init__. Corrected the previously-incorrect urlEqual function.
Commits on Sep 25, 2013
  1. @marky1991

    Corrected the archives.py's get_previous_next_published function. (My…

    marky1991 authored
    … previous corrections were bad.) Finally have gotten rid of all errors except for the querycount-related failures.
  2. @marky1991

    Moved is_before_1_6 to zinnia.utils . I liked it better where it was,…

    marky1991 authored
    … but apparently it causes issues there.
  3. @marky1991
  4. @marky1991

    Made pep8 happy.

    marky1991 authored
  5. @marky1991
  6. @marky1991

    Closed a parenthesis I broke in the previous commit and fixed some la…

    marky1991 authored
    …st pep8 errors. Coverage seems to pass too, so hopefully this works...
  7. @marky1991

    Oops. Removed some debugging files that I thought I had already remov…

    marky1991 authored
    …ed. Everything should be clean and in order now.
  8. @marky1991
Something went wrong with that request. Please try again.