-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
added config options for Woopra #100
base: main
Are you sure you want to change the base?
Conversation
config option as per https://www.woopra.com/docs/setup/javascript-tracking/ added as WOOPRA_XXXX (uppercase of the config name)
variables['hide_campaign'] = str(settings.WOOPRA_HIDE_CAMPAIGN) | ||
except AttributeError: | ||
pass | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment on whether this PR makes sense, because I don't know and use Woopra. So, I focus on Python and documentation:
- The excessive
try
-except
is not DRY and makes the code less readable. Would you mind changing the implementation into a dictionary and a loop, e.g.
woopra_settings = {
'idle_timeout': 'WOOPRA_IDLE_TIMEOUT',
'cookie_name': 'WOOPRA_COOKIE_NAME',
'cookie_domain': 'WOOPRA_COOKIE_DOMAIN',
'cookie_path': 'WOOPRA_COOKIE_PATH',
'cookie_expire': 'WOOPRA_COOKIE_EXPIRE',
'ping': 'WOOPRA_PING',
'ping_interval': 'WOOPRA_PING_INTERVAL',
'download_tracking': 'WOOPRA_DOWNLOAD_TRACKING',
'outgoing_tracking': 'WOOPRA_OUTGOING_TRACKING',
'outgoing_ignore_subdomain': 'WOOPRA_OUTGOING_IGNORE_SUBDOMAIN',
'download_pause': 'WOOPRA_DOWNLOAD_PAUSE',
'outgoing_pause': 'WOOPRA_OUTGOING_PAUSE',
'ignore_query_url': 'WOOPRA_IGNORE_QUERY_URL',
'map_query_params': 'WOOPRA_MAP_QUERY_PARAMS',
'hide_campaign': 'WOOPRA_HIDE_CAMPAIGN',
}
for key, name in woopra_settings:
try:
variables[key] = str(getattr(settings, name))
except AttributeError:
pass
- The settings the code reads are not documented in the Woopra section of our docs. Would you mind adding them to the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@retosteffen Would you be interested in taking this PR forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi.
Sorry for dropping the ball on this but I finally went with a mix of the official woopra js code + woopra python api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But your changes to the Woopra integration, do they make sense?
If yes, would you invest a few minutes to finish this up and push the changes, and we merge everything? This way more people could profit from your contribution.
If not, simply close this PR. So we clean up the PR list.
Anyone interested in having these changes merged? Please, give a 👍. |
config option as per
https://www.woopra.com/docs/setup/javascript-tracking/
added as WOOPRA_XXXX (uppercase of the config name)