docs: Provide more upfront info on features and integrations, nitpicky fixes #345
Conversation
Some integrations do not work out of the box, but this isn't noted anywhere except in the integration's own documentation. Mentioning gotchas with features when they are introduced to the user should help out with this.
relaxolotl
changed the title
| it to the current [`Hub`]. | ||
|
|
||
| The [`sentry::init`] function returns a guard that when dropped will flush Events that were not | ||
| yet sent to the sentry service. It has a two second deadline for this so shutdown of |
There was a problem hiding this comment.
(because seeing this comment anywhere else then in a side-by-side diff makes little sense: yes this is a comment on single-vs-double space sentence ending 😄 )
There was a problem hiding this comment.
😢 I'm glad you pointed this out actually, this is actually a bit of a dilemma for me: Some places in the docs use double spaces, which some auto-formatter that I need to hunt down has been aggressively converting to single spaces. Should I be using 2 spaces, or 1 space?
There was a problem hiding this comment.
honestly, i think it doesn't matter and personally I'm fine with mixed. and if we want consistency we should have an auto-formatter running in CI. I just found it funny that you changed them, but if it was auto-formatted that explains :)
sentry/README.md
Outdated
| | `native-tls` | ✅ | | | `reqwest` must be enabled. | | | ||
| | `rustls` | | | | `reqwest` must be enabled. `native-tls` must be disabled via `default-features = false`. | | | ||
| | `curl` | | | | | | ||
| | `surf` | | | | | |
There was a problem hiding this comment.
You might want to consider a little more visual alignment in this table to make editing this easier. I guess it's fine if the lines end up really long with the comments. And also fully visually aligning is probably not helpful, I'd only try to align the first 4 columns.
There was a problem hiding this comment.
a markdown formatter will take care of all of this ideally.
sentry/README.md
Outdated
| | `slog` | | 🔌 | | | Requires additional setup; See [`sentry-slog`]'s documentation. | | ||
| | `reqwest` | ✅ | | | | | ||
| | `native-tls` | ✅ | | | `reqwest` must be enabled. | | | ||
| | `rustls` | | | | `reqwest` must be enabled. `native-tls` must be disabled via `default-features = false`. | | |
There was a problem hiding this comment.
I think there's an extra trailing column here
|
I wonder why weekly/periodic CI has not been picking up on that lint... we should debug that sometime. Maybe compare with symbolicator's periodic CI. |
There was a problem hiding this comment.
This is awesome, BUT:
All but the top-level readme are actually generated via cargo-readme (+ some custom shell script logic) from the crates top-level rustdoc.
So just make sure to copy over all your changes to the rustdocs instead, and please have the markdown table auto-formatted ;-)
sentry/README.md
Outdated
| | `test` | | | | | | ||
| | `debug-images` | | 🔌 | | | | ||
| | `log` | | 🔌 | |Requires additional setup; See [`sentry-log`]'s documentation. | | ||
| | `debug-logs` | | | ✂️ | | Requires additional setup; See [`sentry-log`]'s documentation. | |
There was a problem hiding this comment.
This is not really an integration per-se, it is rather a compile-time switch to have sentry output some internal logs to the log crate.
sentry/README.md
Outdated
| | `native-tls` | ✅ | | | `reqwest` must be enabled. | | | ||
| | `rustls` | | | | `reqwest` must be enabled. `native-tls` must be disabled via `default-features = false`. | | | ||
| | `curl` | | | | | | ||
| | `surf` | | | | | |
There was a problem hiding this comment.
a markdown formatter will take care of all of this ideally.
sentry/README.md
Outdated
| ### Logging | ||
| - `log`: Enables support for the `log` crate. | ||
| - `slog`: Enables support for the `slog` crate. | ||
| - `debug-logs`: **Deprecated**. Uses the `log` crate for internal logging. |
There was a problem hiding this comment.
that would be nice. but we should run it in CI in that case
There was a problem hiding this comment.
just so i have this sorted out: sentry-core's debug-logs feature is deprecated according to its docs: https://github.com/getsentry/sentry-rust/blob/master/sentry-core/README.md#features
debug-logs on sentry specifies sentry-core's debug-logs as a dependency. is debug-logs on sentry-core deprecated, but not on sentry itself?
Codecov Report
@@ Coverage Diff @@
## master #345 +/- ##
==========================================
+ Coverage 83.08% 86.59% +3.51%
==========================================
Files 62 66 +4
Lines 6478 6842 +364
==========================================
+ Hits 5382 5925 +543
+ Misses 1096 917 -179 |
|
Thank you for the feedback, especially the part where the READMEs are actually automatically generated; I'd completely missed that. All of the changes have been migrated over to the rustdocs. The table has also been run through a markdown formatter, but I reckon it would be preferable if we used an extension to enforce this. I wasn't able to find any extensions that target this specific use case with a quick search, so any guidance or advice on what to do here would be great. While resolving the lints I'd noticed that some of our links to integrations pointed to nonexistent entries due to feature flags. I've opened up a separate PR to address that linter warning which should also expose previously-hidden bits of the API which were gated behind feature flags: #346
If I understand this correctly, it looks like the weekly CI has been complaining for a while about this, actually: https://github.com/getsentry/sentry-rust/actions/runs/826285550 from a month ago is the first time it started throwing a fit and the weekly CI runs have been failing since that run. 1.53.0 was just stabilized last week which included this check, so perhaps this is more of a visibility/notification issue than it is a CI problem? |
…y fixes (getsentry#345) Some integrations do not work out of the box, but this isn't noted anywhere except in the integration's own documentation. Mentioning gotchas with features when they are introduced to the user should help out with this.
Adds some extra info in crate docs that should smooth out the initial setup process without bloating the quickstart section. This includes some information about integrations that do not work out of the box and require additional setup to function, as suggested in #343.
I've taken the liberty to try to lay everything out in a table in hopes that this makes things easier to parse at a glance. The table notes which features are enabled by default, as well as ones that are deprecated (?) which will most likely need to be double-checked. Most of these changes reside in
/sentry/README.md, and everything else is mostly composed of nitpicky fixes that I noticed while I double-checked to make sure all of the docs were consistent.