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

Bug 1372640 - Add django-filters to INSTALLED_APPS #2575

Merged
merged 1 commit into from
Jun 19, 2017

Conversation

SJasoria
Copy link
Contributor

No description provided.

Copy link
Contributor

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Many thanks for the PR!

Could you update the commit message to:

  • Describe what was changed rather than a symptom of the bug (if you haven't seen it before https://chris.beams.io/posts/git-commit/ is a good read)
  • In the multi-line part of the commit message, reference the Django filters installation guide and why this change is necessary

So perhaps something like:

Bug 1372640 - Add django-filters to INSTALLED_APPS

The jobdetail browsable API pages were returning HTTP 500 since as of
carltongibson/django-filter#580 it's necessary to add django-filter to
INSTALLED_APPS:
https://django-filter.readthedocs.io/en/latest/guide/install.html

Many thanks!

@@ -153,6 +153,7 @@ def server_supports_tls(url):
'treeherder.autoclassify',
'treeherder.credentials',
'treeherder.seta',
'django_filters'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put this above the Treeherder entries and below the other custom Django packages (eg perhaps between corsheaders and graphene_django). Also in Python trailing commas are allowed in dicts/lists/... and are often preferred, since it reduces churn when adding an extra line to the end :-)

@SJasoria SJasoria changed the title Bug 1372640 - Treeherder jobdetail API throwing 500 when hit via web browser Bug 1372640 - Add django-filters to INSTALLED_APPS Jun 19, 2017
The jobdetail browsable API pages were returning HTTP 500 since as of
carltongibson/django-filter#580 it's necessary to add django-filter to
INSTALLED_APPS: https://django-filter.readthedocs.io/en/latest/guide/install.html
Copy link
Contributor

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Many thanks!

@edmorley edmorley merged commit d9045d0 into mozilla:master Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants