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

Fix django 2.1 methods render #58

Merged
merged 1 commit into from
Feb 24, 2019
Merged

Fix django 2.1 methods render #58

merged 1 commit into from
Feb 24, 2019

Conversation

dubirajara
Copy link
Contributor

Fix to Django 2.1 release Support for Widget.render ( ) methods without the renderer argument is removed.

close #56

Fix to Django 2.1 release Support for Widget.render ( ) methods without the renderer argument is removed.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.727% when pulling ea13f2a on dubirajara:develop into 41b836a on radiac:develop.

@Fiona
Copy link

Fiona commented Nov 16, 2018

Aaaagh I really really wish this was merged in. :(

@4levels
Copy link
Contributor

4levels commented Nov 21, 2018

@radiac Have you abandoned this project? 2 perfectly valid PR's with no response whatsoever..

@radiac
Copy link
Owner

radiac commented Nov 23, 2018

Sorry for the delay looking at these - I appreciate all contributions, and am very aware these two are waiting on me, but I'm really busy with my day job and personal life at the moment, and simply haven't had time.

I may be paranoid but I don't want to merge and release without investigating and testing myself first. For example, this PR needs tox and travis changes to test against 2.0 and 2.1, which may throw up some more issues. The other branch doesn't include a test, so I'll need to write one which fails before the PR and passes after. I'd expect this will take a couple of hours - hopefully I'll find the time soon.

So, I'll get to this when I can; in the meantime if there's a branch that you want to use which I haven't got round to merging you can install that directly with

pip install -e git+https://github.com/<user>/django-tagulous.git@<branch>#egg=django-tagulous

@dubirajara
Copy link
Contributor Author

hi @radiac, sorry for trying to contribute without having looked at the instructions on how to contribute to the project, for the next i try to make a complete pr with test. Thank you

@4levels
Copy link
Contributor

4levels commented Nov 23, 2018

Really? A new test for a keyword argument that didn't exist before and now has to be passed as None due to Django's updated render method signature?

I mean, I'm a great fan of a fully tested codebase as well, but this argument does not apply here IMO. @radiac did you even look at the diff of this PR? Come on man, just merge this trivial change, you've alread spent way more time apologzing why you didn't merge it yet ;-)

Regarding my own PR #61 (division by zero / quoted tablenames) I couldn't agree more that a new test is required. But since I'm still considering myself a Django / Python beginner I don't even know where to start. I can only confirm that I'm runing my fork in production for quite some time now without any issues.

@radiac
Copy link
Owner

radiac commented Nov 23, 2018

@dubirajara No problem at all - your patch doesn't really need a test in itself, I just want to merge it as part of the next release to support dj2.0 and dj2.1 - that's the bit I'll need to test! Thanks for your contribution, it's clearly in demand - I hope to have a chance to merge it soon.

@4levels I do appreciate your support, and understand your frustration. However this project is particularly vulnerable to internal changes between versions of Django, so I don't want to put out a version which might, for example, have an edge case that loses people's data because dj2.0 changed an internal method call and I haven't tested it thoroughly. When I release the next version and I'm telling people who trust this project that dj2.0 and dj2.1 are supported properly, I want to be confident in that statement.

I welcome all contributions, and I don't require that PRs always come with tests as I don't want to put people off - I'm grateful to anyone willing to put effort into helping me make these projects better - but before I merge a PR I do just want to take the time to understand why the change is necessary, what its implications are, and make sure it's going to work for everyone. Unfortunately when I'm busy elsewhere that can mean delays.

Thanks to everyone for your support and continued patience - it's great to know that people are using Tagulous, and I will do my best to get to this as soon as I can.

@radiac radiac merged commit ea13f2a into radiac:develop Feb 24, 2019
@radiac
Copy link
Owner

radiac commented Feb 24, 2019

Thanks again for submitting this patch, this has now been merged and is part of the release 0.14.0, which should have full support for django 2.0 and 2.1.

My apologies to everyone that this has taken so long to get out - there were 190 failing tests in dj1.11 -> dj2.1, but we got there eventually. Thanks to everyone for your patience!

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

Successfully merging this pull request may close these issues.

Django 2.1 incompatibility.
5 participants