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

Ref: Add include_source_context option in utils #2020

Conversation

farhat-nawaz
Copy link
Contributor

@farhat-nawaz farhat-nawaz commented Apr 17, 2023

  • Added include_source_context option to serialize_frame function to allow users to opt-out of getting source context.

This is my very first open-source contribution. Please let me know if I missed something.

Fixes #2017

Some users do not like the source context to be there, and so add `include_source_context` option to opt-out.

Fixes getsentry#2017
@farhat-nawaz farhat-nawaz force-pushed the farhat-nawaz/add-include-source-context-option-in-utils branch 2 times, most recently from c04c8fc to cf3cc22 Compare April 18, 2023 00:09
@farhat-nawaz farhat-nawaz force-pushed the farhat-nawaz/add-include-source-context-option-in-utils branch from cf3cc22 to 6367cbf Compare April 18, 2023 19:25
@farhat-nawaz farhat-nawaz marked this pull request as ready for review April 18, 2023 19:40
@antonpirker
Copy link
Member

Hey @farhat-nawaz !

Thanks for the contribution! Looks good!

Now we also need the include_source_context option in ClientConstructor class at the end and need to pass this option along so that all calls to serialize_frame are given the option set in ClientConstructor. (you can see include_local_variables for a similar option)

@antonpirker
Copy link
Member

If you do not have time it is also possible that we merge this and take over the rest of the implementation. Should we do this?

@farhat-nawaz
Copy link
Contributor Author

@antonpirker Sure, I can add those changes, no problem.

@sentrivana
Copy link
Contributor

Hey @farhat-nawaz, thanks again for contributing! Did you already have time to start on the changes that Anton proposed? If not, I'm happy to take it from here and finish it.

@sentrivana
Copy link
Contributor

Hi @farhat-nawaz, the PR looks good to me, I will merge it now as is and we'll add the missing stuff. Thank you for contributing! 🙏

@sentrivana sentrivana merged commit fbd7d1a into getsentry:master May 11, 2023
244 checks passed
@farhat-nawaz
Copy link
Contributor Author

@sentrivana Glad to have made the contribution, and looking forward to more. Sorry about not being able to respond earlier, something unexpected came up. :)

@sentrivana
Copy link
Contributor

@farhat-nawaz No problem at all, thank you for getting this kick-started!

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.

Add include_source_context option
4 participants