Skip to content

Make hub a required argument for Transaction constructor#1401

Merged
st0012 merged 4 commits intomasterfrom
implement-#1393
Apr 16, 2021
Merged

Make hub a required argument for Transaction constructor#1401
st0012 merged 4 commits intomasterfrom
implement-#1393

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Apr 16, 2021

This implements the change proposed in #1393.

Closes #1393

@st0012 st0012 added this to the 4.4.0 milestone Apr 16, 2021
@st0012 st0012 self-assigned this Apr 16, 2021
@st0012 st0012 requested a review from rhcarvalho April 16, 2021 13:34
@rhcarvalho rhcarvalho changed the title Make hub a required argument for Transaction construbtor Make hub a required argument for Transaction constructor Apr 16, 2021
attr_reader :name, :parent_sampled, :hub, :configuration, :logger

def initialize(name: nil, parent_sampled: nil, hub: Sentry.get_current_hub, **options)
def initialize(name: nil, parent_sampled: nil, hub:, **options)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@st0012 I'm learning Ruby from you, thanks!

I just tried this out on irb. So if hub is not set, there's an error:

ArgumentError (missing keyword: hub)

Nice!

Not in the scope of this PR, but wanted to share that this could as well be the behavior for name. Transactions must have a name. Deciding if it is required or not in the constructor is debatable though, sometimes you don't know the name or need to update it later before sending the transaction. In some languages we write a debug message if the name is not set right before sending the transaction to Sentry.

If a transaction is sent without a name it gets a default value, but that's not very useful. The transaction name is what is used to group the many different traces.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing out the potential issue. I'll keep that in mind 🙂

@st0012 st0012 merged commit f82a2c3 into master Apr 16, 2021
@st0012 st0012 deleted the implement-#1393 branch April 16, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make hub a required argument in Transaction's constructor

2 participants