Skip to content

refactor commentbox_props() into classmethod to allow better overriding #372

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

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

PetrDlouhy
Copy link
Contributor

I am using the commentbox_props() function in my app to create my own comment view.
I wanted to restrict the queryset in this function to show correct number of comments (not counting comments private to user).

With this slight change of the commentbox_props() function into classmethods I am able to do this without need of copying of the entire code of the function.

@PetrDlouhy
Copy link
Contributor Author

BTW If you want to see the diff without huge amount of noise caused by indentation change, use the "Hide whitespace" function of GitHub:
Snímek obrazovky_2022-07-07_15-36-51

@PetrDlouhy PetrDlouhy force-pushed the comment_props_overridable branch from b74ab2e to cbed69d Compare July 7, 2022 13:42
@PetrDlouhy PetrDlouhy force-pushed the comment_props_overridable branch from cbed69d to 7653462 Compare July 7, 2022 13:57
@danirus danirus merged commit 35415fe into danirus:master Jul 8, 2022
@danirus
Copy link
Owner

danirus commented Jul 8, 2022

Thank you @PetrDlouhy.

Maybe you want to consider sending the same changes (and modifying the existing tests for the frontend.py module) to django-comments-ink. It's still under development, but at some point it will supersede django-comments-xtd.

@PetrDlouhy
Copy link
Contributor Author

@danirus Thanks for merging and improving my PR.

I will look at the -ink repository. Are somewhere summarized the main changes and improvements?
For www.blenderkit.com we ended up using our custom JavaScript frontend written in Svelte/Vite (which translates to pure JavaScript without any additional bindings needed). We probably will not be rewriting that to use django-comments-ink's JavaScript frontend, but I can send you the code if you are interested (or make a separate pluggable app, if I have some spare tiime).

Should I re-post my PRs from here also to django-comments-ink, or you will transfer them there?
Or should I start posting them only there?

I polished #342, so please merge that, I will look at the other rests later.

@danirus
Copy link
Owner

danirus commented Jul 12, 2022

Thanks for the offer of the Svelte plugin, but I have very little spare time. It would nice if you made it open source and people could install it with npm or yarn. :-)

In django-comments-ink the major differences with -xtd are that:

  1. It does not depend on any JavaScript framework. The plugin is vanilla JavaScript.
  2. There is a new CommentReaction model (and ObjectReaction) that is customizable.
  3. There is no tree of comments but a rather plain QuerySet that is rendered as a tree. The render_xtdcomment_tree is no longer available, and it's been replaced by render_inkcomment_list tag.
  4. Comments can be paginated.
  5. A lot of tests.

I'm adding more features at the speed of a snail.

It would be great if you could bring there the changes you suggested about using UUIDs for comment PKs. I remember that it had little explanation, it looked like a hack. If you don't want to spend time writing literature, do you have an open public repository using django-comments and django-comments-xtd with UUIDs to illustrate the case?

I will look into #342 at the end of the week.
Thanks for your contributions!

@PetrDlouhy
Copy link
Contributor Author

@DeniRus Using UUIDs as PKs itself is almost no problem with current implementation. The only change needed are few forgotten id instead of pks, which I fixed in #342.

What is more challenging is setting comment URLs to non-PK field, which is needed if you want to add UUID to current model which is using IDs and don't want to rewrite half of the application and end up in migration hell.
I did use proxy models to achieve that. I did few hacks on the side of my application, and few non-hack modification on the side of django-comments-xtd.

I don't have public implementation, but I can transform the code into documentation or sample application.

I might also try to explore more another solution - to have settings with field mappings. Something like:

COMMENTS_ID_OVERRIDES = {
   'Article': 'my_uuid_field',
   'Item': 'another_id_field',
}

But I must say, that I did dive into that before, and I was not able to implement it. One of the reason is, that it did require many modifications also at the side of django-comments-contrib.

@PetrDlouhy
Copy link
Contributor Author

@danirus I have completed that taught in django/django-contrib-comments#188 and #374.
I think, that this solution is much cleaner and easier for implementors. But it relies on the changes being accepted in django-contrib-comments. So I will first have to finish the discussion and work on PR there and then I can polish the PR here.

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.

2 participants