-
Notifications
You must be signed in to change notification settings - Fork 41
Django example app - expanding & updating #139
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
Conversation
|
a minor update to the notifier is needed to make this app compatible with the latest version of Django, but let me know any comments on the app itself. |
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.
Mostly nit-picking and stylistic changes, demo looks good!
On a larger note maintaining the older Django example would ensure the tests can be run against both Django 1 and 2. This could probably do with some discussion as integration tests are in the process of changing at the moment.
example/django/README.md
Outdated
| See `demo/views.py, #crash_with_callback` | ||
| 1. Run the application. (Make sure to use any 3+ version of python.) | ||
| ```shell | ||
| python3.6 manage.py runserver |
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 don't know that we want to go this granular with the python command, I think python would be better than python3.6
example/django/README.md
Outdated
| Basically, almost any unhandled exception sends a notification to Bugsnag. | ||
| Pressing this link would lead to an empty page, which is normal. | ||
| See `demo/views.py, #crash` | ||
| 1. Before testing it, open up the `settings.py` |
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.
settings.py is found in bugsnag_demo, would it be worth pointing directly to the file i.e. bugsnag_demo/settings.py
|
|
||
| MIDDLEWARE_CLASSES = ( | ||
| MIDDLEWARE = ( | ||
| # make sure to add Bugsnag to your middleware. |
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.
While this step is mentioned in the getting started docs, I think it can be overlooked and it may deserve a step in the readme.
example/django/index.html
Outdated
| </a> | ||
| <br> | ||
| <a href="/notifywithcontext"> | ||
| <button type="button" id="typeError">MANUAL NOTIFICATION</button> |
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.
MANUAL NOTIFICATION doesn't give a great indication of what the button is actually doing. It would be good to differentiate between that and HANDLED ERROR.
example/django/index.html
Outdated
| <br> | ||
| <br> | ||
| <div id="python"> | ||
| <a href="crash/"> |
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.
Stray underlines can be fixed with style="text-decoration:none;"
example/django/README.md
Outdated
| "api_key": "066f1ad3590596f9aacd601ea89af845" | ||
| } | ||
| ``` | ||
| Please note that while bugsnag-python is compatible with previous Python 2 and Django 1 versions -- however, this particular example app will only work with Python 3+ and Django 2+, to showcase the most current configurations. See [our documentation](https://docs.bugsnag.com/platforms/python/django/) for further details. |
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 think this is a good idea, but I also think it'd be good to maintain a Django 1.11 example, maybe in a django_legacy folder?
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.
If anything, I'd call it django1.11 for clarity
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.
sounds good! I had built one, but then worried it would be too redundant. I'll rename it and submit in a separate PR.
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.
added django 1.10 in this same PR.
|
thanks for the detailed notes! I believe my latest commits address all of them. |
|
Looks good! I'd like to change the example directory name to |
|
This is probably going to require refactoring the tests from using the examples to using proper test-fixtures, which is going to take a little longer. This is still active though! |
|
Merged as a part of this PR |
Added more detailed configuration examples. Updated to Django2 with Python3. (not backwards compatible, due to Django's dependency.)