Cleanup, example and overall improvements to django-threadedcomments #39
Conversation
Bugfix/last child on delete
Seems like a neat thing to do before merging code.
- fixes deleting a parent when deleting the last child. - checks for the 'for' keyword in the template tag. Conflicts: threadedcomments/migrations/0001_initial.py (South 0.7.4 with Django 1.4 generates different frozen models)
Also include a warning that South 0.7.4 (current version) does not recognize the on_delete=SET_NULL option yet.
Synced settings with 1.4 defaults, to include staticfiles and new template loader definitions. Made adjustments to be 1.3 and 1.4 compatible; - excluded LOGGING configuration, which differs between 1.3 and 1.4 - and included the ADMIN_MEDIA_PREFIX
Use context processors, jQuery from a CDN (avoid inclusion just for the example), and cleaned up JavaScript with 'cancel reply' function. Split templates to base.html and home/message.html
Can't remember seeing someone behind a VT100 terminal for a very long time.. ;-)
Considering that 2.5 is even unsupported, other projects dropped it much sooner last year, and even Django dropped it in 1.4, it's about time. Using new @decorator syntax now.
Had a hard time understanding the code, improved that.
- Include syntax examples in if-statements. - Add docstrings for the `annotate_tree` and `fill_tree` filters.
The code operates just like the version of `django.contrib.comments`,
and also supports the `flat` and `root_only` flags like
`get_comment_list` does.
With these 2 tags, the `{% load comments %}` tag is swappable
with `{% lad threadcomments_tags %}`
Allow to be used by any `get_whatever` node
Copied `get_object` / `get_form` logic from django.contrib.comments.
Keeping inline example as separate template.
This is required to let the default absolute URL of Comment work.
This is what the example app is for after all :-)
| @csrf_protect | ||
| def message(request, id): | ||
| context = { | ||
| 'message': Message.objects.get(id=id), |
There was a problem hiding this comment.
Could you do get_object_or_404 here please
|
wow, thanks a lot! I have planned on doing a tiny sprint on threadedcomments this Tuesday so this is absolutely perfect. I will add a few comment on the code on what I spot, but overall it looks very good. Thanks! |
| DATABASES = { | ||
| 'default': { | ||
| 'ENGINE': 'django.db.backends.sqlite3', | ||
| 'NAME': os.path.join(os.path.dirname(__file__), 'sampledb.db'), |
There was a problem hiding this comment.
better than including a sqlite file would be providing some fixtures so peaple can use whatever (also no git conflicts etc)
There was a problem hiding this comment.
Good point. Didn't consider the possible conflicts. I've added a example-data.json.
I did find it convenient, most Django apps lack an example making it hard to evaluate them.
Just having to do a download + runserver was a breeze to get a good preview.
Nicely suggested by @honzakral in the comments of pull #39
Avoids git conflicts and allow testing other databases faster. Nice suggestion by @honzakral in git pull #39
|
...great to know the timing was that good, and the work in appreciated :-) |
|
well, we are having a little django sprint here in Prague, but since tickets have been piling up here I thought I would invest some effort into cleaning things up, your work is a great help. |
|
hey, thx for this huge amount of work. it looks like you invested a lot of time into this project. i am willing to merge this stuff soon, but right now, i will test it in our production. if this integration test pass, lets make it a release, ok? before that i have to check what version actually is on pypi.. can i have one {prosbu} for you? next time you want us to pull something, just split it to more pull requests - i understand we are way behing with new stuff - but it is way easier to check and commit smaller changes. thx again, looking forward for more cleanup and features from you ;)) |
|
Seems like a good plan, especially the testing and making a release as well. :-) I'll keep my pull requests smaller next time. This was indeed a massive sprint to get things into shape, I don't think such big change is needed in the future. If you have any specific feature plans in mind, please add them to the README. It's always nice to know where a project is heading to, and I'll keep the invitation in mind ;-) |
|
Hi, did you find time to merge the pull request back in? I was hoping to see a release on pypi :-) |
|
hey @vdboor - sorry, i didn't have anymore time. but we actually had another small sprint on tuesday. we finally have access to pypi, so right now, we will release current version there and your changes will follow asap. |
|
hi @kvbik, thanks for the follow up! This is great news to hear! |
|
sorry guys, I don't use MySQL so I haven't had a chance to test it properly, I will get right on it. |
|
@honzakral: ok, very cool! |
|
This is merged, Thanks a lot and sorry for the delay |
Using comments/list.html now, like the pull request at honzakral/django-threadedcomments#39 provides.
Hi,
I've been looking into getting django-threadedcomments up to date, with clean examples. I hope you accept this first series of changes. It contains:
render_comment_listtag andget_comment_countso that{% load comments %}can be replaced by{% load threadedcomments %}These were all parts I missed in the application, and I hope it saves other peoples time too.
Thanks for your time to include these changes.
Diederik