Skip to content

Ref: Add include_source_context option in utils#2020

Merged
sentrivana merged 8 commits intogetsentry:masterfrom
farhat-nawaz:farhat-nawaz/add-include-source-context-option-in-utils
May 11, 2023
Merged

Ref: Add include_source_context option in utils#2020
sentrivana merged 8 commits intogetsentry:masterfrom
farhat-nawaz:farhat-nawaz/add-include-source-context-option-in-utils

Conversation

@farhat-nawaz
Copy link
Copy Markdown
Contributor

@farhat-nawaz farhat-nawaz commented Apr 17, 2023 •
edited
Loading

  • 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

@farhatnawaz
ref: Add include_source_context option
e61b1e7 
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
Copy Markdown
Contributor

antonpirker commented May 4, 2023

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
Copy Markdown
Contributor

antonpirker commented May 4, 2023

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
Copy Markdown
Contributor Author

farhat-nawaz commented May 4, 2023

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

@sentrivana
Copy link
Copy Markdown
Contributor

sentrivana commented May 8, 2023

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
Copy Markdown
Contributor

sentrivana commented May 11, 2023

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
@sentrivana sentrivana mentioned this pull request May 11, 2023
Merged
@farhat-nawaz
Copy link
Copy Markdown
Contributor Author

farhat-nawaz commented May 24, 2023

@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
Copy Markdown
Contributor

sentrivana commented May 25, 2023

@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

Reviewers

@sentrivana sentrivana sentrivana approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add include_source_context option

4 participants

@farhat-nawaz @antonpirker @sentrivana @farhatnawaz