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

get rid of SITE_ID (for multiple site installation) #125

Merged
merged 1 commit into from Sep 30, 2019

Conversation

gassan
Copy link
Contributor

@gassan gassan commented Apr 28, 2019

subj

@danirus
Copy link
Owner

danirus commented Apr 30, 2019

Thanks @gassan for taking the time to do this.

I would prefer not to wrap django.contrib.sites.shortcuts.get_current_site. I haven't seen any Django project using this package without django.contrib.sites installed, however I think it's safe to have the fallback you added when there's no pk attribute.

Did you consider getattr(get_current_site(request), "pk", 1) instead of creating utils.get_site_id?

The CI issues are most probably because you go beyond column 79. I would appreciate if you could keep that constraint.
Thanks again.

@gassan
Copy link
Contributor Author

gassan commented May 2, 2019

Did you consider getattr(get_current_site(request), "pk", 1) instead of creating utils.get_site_id?

Good idea - I will use getattr and fix line lengths later this week

def _reverse: avoid http absolute urs on https pages
@gassan
Copy link
Contributor Author

gassan commented May 2, 2019

I need the shortcut utils.get_current_site_id - this function is called from 11 places.

@gassan
Copy link
Contributor Author

gassan commented May 3, 2019

use pip<19 for install from github.
pip install git+https://github.com/gassan/django-comments-xtd.git@nositeid

@@ -46,14 +47,14 @@ def commentbox_props(obj, user, request=None):
"""

def _reverse(*args, **kwargs):
"""Inject the request, if provided, to generate absolute URLs"""
return reverse(*args, request=request, **kwargs)
"""do not inject request to avoid http:// urls on https:// only sites"""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an observation here, if no request is passed to reverse here, there's no point on keeping the _reverse function. You can replace all the calls to _reverse with the original reverse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes you are right here

@danirus danirus self-assigned this Aug 26, 2019
@danirus danirus merged commit 1b4af66 into danirus:master Sep 30, 2019
@danirus
Copy link
Owner

danirus commented Sep 30, 2019

Thanks @gassan,
and sorry for the long delay.

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

2 participants