Skip to content

Conversation

@Cawllec
Copy link
Contributor

@Cawllec Cawllec commented May 9, 2018

From this PR by @tremlab, this addition changes the Django tests to no longer rely on the Django example, but on a django app in the test/fixtures folder, which was stopping the previous PR from being mergeable.

I realise the linter is failing this at the moment, so will be investigating why ASAP.

@Cawllec Cawllec requested a review from a team May 9, 2018 16:33
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`](bugsnag_demo/settings.py)
file and configure your API key.
Copy link
Contributor

@fractalwrench fractalwrench May 10, 2018

Choose a reason for hiding this comment

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

I opened up this file and saw SECRET_KEY, which could potentially be a bit confusing (for PyNoobs). Could we define a similar constant for the API key at the top of the file also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a Django feature. To avoid confusion I could modify to:

Before testing it, open up the [`settings.py`](bugsnag_demo/settings.py)
file and configure your API key within the `BUGSNAG` dictionary.

python manage.py runserver
```

1. View the example page which will (most likely) be served at: http://localhost:8000
Copy link
Contributor

Choose a reason for hiding this comment

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

What circumstances would cause it not to be served on 8000? Do we know what the port would be in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user can change the port when starting the server, although I agree on the language change, perhaps to which, by default, will be served at:

`info`.
See `demo/views.py, #severity`
For more information, see our documentation:
https://docs.bugsnag.com/platforms/python/django/
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to use a markdown link for this?

when notifying bugsnag of the error. Valid severities are `error`, `warning` and
`info`.
See `demo/views.py, #severity`
For more information, see our documentation:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding an extra step here for unfamiliar users, stating that you can send errors and see them immediately show up on dashboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's mentioned above at line 9, and also mentioned in the messages received from the server when calling the functionality. Would it be a good idea to link to app.bugsnag.com at the bottom of the README?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's mentioned in the server messages, then I don't think this requires any change


STATIC_URL = '/static/'

# Initialize Bugsnag to begin tracking errors. Only an api key is required, but here are some other helpful configuration details:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very useful!

# By default, requests are sent asynchronously. If you would like to block until the request is done, you can set to false.
"asynchronous": False,

# if you track deploys or session rates, make sure to set the correct version.
Copy link
Contributor

Choose a reason for hiding this comment

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

if -> If

```
1. Clone the repo and `cd` into this directory:
```shell
git clone https://github.com:bugsnag/bugsnag-python.git
Copy link
Contributor

@fractalwrench fractalwrench May 10, 2018

Choose a reason for hiding this comment

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

This doesn't work (there's a colon instead of a forward slash)

url(r'^crashcallback/$', crash_with_callback),
url(r'^handled/$', handled),
url(r'^notifywithcontext/$', context),
url(r'^notifywithmetadata/$', notify_meta),
Copy link
Contributor

Choose a reason for hiding this comment

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

The old example app seems to have a severity endpoint - is this concept demonstrated to users in one of the above URLs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this, I'll query it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the context endpoint now modifies the severity.

<button type="button">NOTIFICATION WITH METADATA</button>
</a>
</div>
<!-- review settings.py & views.py to see how bugsnag integrates with your app. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

If this message is necessary, it would be better as an element than a comment so it's visible when editing or viewing in the browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't, it's just a comment for people browsing the html.

Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

Left a few minor comments inline. I really like the level of inline docs/pydoc in the examples. The one comment that definitely needs addressing is a typo in the setup instructions that means git clone doesn't work out-of-the-box.

@Cawllec Cawllec changed the base branch from next to master May 10, 2018 14:14
@Cawllec
Copy link
Contributor Author

Cawllec commented May 10, 2018

I've changed the target to Master as this is primarily an examples update and should reflect on the main repository.

@fractalwrench fractalwrench self-requested a review May 11, 2018 09:15
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

This looks good to me assuming it has been tested by Alex/Renee. Approving with the provision that we should inform the relevant people at PyCon before merging this.

@Cawllec
Copy link
Contributor Author

Cawllec commented May 11, 2018

Sounds good. As soon as Renee is online I'll inform her and merge afterwards.

@Cawllec Cawllec merged commit 7c0f794 into master May 14, 2018
@Cawllec Cawllec deleted the tremlab-django_ex branch May 14, 2018 08:35
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.

4 participants