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

docs: Rename python configuration options and add deprecation alerts #7432

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

mgaligniana
Copy link
Contributor

@mgaligniana mgaligniana commented Jul 13, 2023

Rename request_bodies to max_request_body_size and add deprecation alert.
Add deprecation alert also for with_locals.

Pre-merge checklist

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs
  • PR was reviewed and approved by a member of the Sentry docs team

Description of changes

This PR adds the documentation for the changes made in getsentry/sentry-python#2247 for ticket getsentry/sentry-python#926.

A preview of how it looks like:

image

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Extra resources

@vercel
Copy link

vercel bot commented Jul 13, 2023

@mgaligniana is attempting to deploy a commit to the Sentry Team on Vercel.

A member of the Team first needs to authorize it.

@@ -379,6 +396,12 @@ The number of characters after which the values containing text in the event pay

<ConfigKey name="with-locals" supported={["python"]}>
Copy link
Contributor Author

@mgaligniana mgaligniana Jul 13, 2023

Choose a reason for hiding this comment

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

I noticed this option will also be deprecated: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/client.py#L82C20-L82C20 so I added an alert here too!

@vercel
Copy link

vercel bot commented Jul 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 18, 2023 9:20pm

Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Thanks for adding the change to the docs! Looks good to me text-wise, what I think we can improve on a bit is the organization.

Right now the config key list is a mixture of deprecated and supported options. There's a #### Deprecated heading a bit further down the page, we should move the request-bodies and with-locals there. I'd also remove the description for those and just leave the deprecation note there with the link to the new config option.

@mgaligniana mgaligniana force-pushed the sentry_python_ticket_2247 branch 2 times, most recently from caa9ccd to 2996ec1 Compare July 14, 2023 12:04
@mgaligniana
Copy link
Contributor Author

mgaligniana commented Jul 14, 2023

I moved the two options to the bottom and added the "deprecated" header (I removed the text because I think is "repetitive", what do you think?) -> in the PR description I updated an image how it looks now!

And I updated the diff too! I realized python was the last one to move inside of supported=, because the max-request-body-size description already exists.

Thank you!

@sentrivana
Copy link
Contributor

I moved the two options to the bottom and added the "deprecated" header (I removed the text because I think is "repetitive", what do you think?) -> in the PR description I updated an image how it looks now!

Looks great!

And I updated the diff too! I realized python was the last one to move inside of supported=, because the max-request-body-size description already exists.

Nicely spotted :) Yes, that makes sense.

Looks good to me, handing this over to our docs team.

@sentrivana
Copy link
Contributor

@mgaligniana Just noticed we reference request_bodies on this page as well, could you please update it to the new name there?

@mgaligniana
Copy link
Contributor Author

Ohh, good catch! Sure!

Rename `request_bodies` to `max_request_body_size` and
add deprecation alert. Add deprecation alert also for `with_locals`.
@antonpirker antonpirker merged commit 08e4abe into getsentry:master Jul 19, 2023
6 checks passed
shanamatthews pushed a commit that referenced this pull request Jul 31, 2023
…7432)

Rename `request_bodies` to `max_request_body_size` and
add deprecation alert. Add deprecation alert also for `with_locals`.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants