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

Reordered parameters to TransactionContext and SpanContext constructors #2696

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Oct 4, 2023

Consolidated multiple TransactionContext constructors into a single constructor with default values for most of the arguments... so all the convience of some overloads and all the power of others.

SDK Change Notes

Both the TransactionContext and SpanContext classes now have default values for most of the values that can be passed to their constructors, which should mean you no longer need to provide unecessary null values etc. for arguments you don't care about.

However in order to make this change, the order of parameters had to be changed so that the non-optional parameters come first in the constructor method signatures. If you're constructing instances of these classes in your own code (which is unlikely), you will need to adjust the order in which you pass parameters to these. See the PR changes for details.

@jamescrosswell jamescrosswell self-assigned this Oct 4, 2023
@jamescrosswell jamescrosswell added this to the 4.0.0 milestone Oct 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 7e08a92

@jamescrosswell jamescrosswell marked this pull request as ready for review October 9, 2023 00:46
Copy link
Contributor

@bitsandfoxes bitsandfoxes 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 pulling all those together!

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

didn't really look at the change, sorry but another nit on the changelog: this is a breaking change and ppl become afraid of taking them sometimes. It's helpful when we give folks a migration path that's clear to follow

CHANGELOG.md Outdated Show resolved Hide resolved
@jamescrosswell jamescrosswell changed the title Removed TransactionContext constructor overloads Consolidated TransactionContext constructors and reordered parameters to TransactionContext and SpanContext constructors Oct 11, 2023
@jamescrosswell jamescrosswell changed the title Consolidated TransactionContext constructors and reordered parameters to TransactionContext and SpanContext constructors Reordered parameters to TransactionContext and SpanContext constructors Oct 11, 2023
@jamescrosswell jamescrosswell merged commit 7e12c2b into feat/4.0.0 Oct 11, 2023
14 checks passed
@jamescrosswell jamescrosswell deleted the feat/4.0.0-TransactionContext-constructors branch October 11, 2023 22:17
vaind pushed a commit that referenced this pull request Oct 14, 2023
…rs (#2696)

* Replaced multiple TransactionContext constructors to a single constructor having default properties
* Made most of the SpanContext constructor parameters optional
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.

3 participants