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

Fix variable names in rescue blocks #481

Merged
merged 2 commits into from
Jul 13, 2018
Merged

Conversation

kitop
Copy link
Contributor

@kitop kitop commented Jul 10, 2018

When running the gem in production, we came across the following error:

09:24:55.681 .../bundle/ruby/2.5.0/gems/ddtrace-0.13.0/lib/ddtrace/workers.rb:46:in `rescue in callback_traces': undefined local variable or method `items' for #<Datadog::Workers::AsyncTransport:0x000055cecd711640> (NameError)
09:24:55.681 .../bundle/ruby/2.5.0/gems/ddtrace-0.13.0/lib/ddtrace/workers.rb:38:in `callback_traces'
09:24:55.681 .../bundle/ruby/2.5.0/gems/ddtrace-0.13.0/lib/ddtrace/workers.rb:111:in `block in perform

And noticed that this commit changed the variable name in the method body but not in the rescue block.

This PR updates the variable names to the proper ones in rescue blocks.

@delner delner self-requested a review July 10, 2018 22:05
@delner delner added bug Involves a bug core Involves Datadog core libraries community Was opened by a community member labels Jul 10, 2018
@delner delner requested a review from pawelchcki July 11, 2018 17:21
@delner
Copy link
Contributor

delner commented Jul 11, 2018

Hey @kitop, thanks for the PR! This looks like a pretty clear cut bug, and the fix looks good. It'd be a good idea to add an RSpec test that replicates this issue so we can confirm the efficacy of the change, and prevent any regressions. Once we have one of those, I think we can merge this one.

@kitop
Copy link
Contributor Author

kitop commented Jul 11, 2018

@delner thanks for the feedback! Will add tests for it.

@kitop
Copy link
Contributor Author

kitop commented Jul 11, 2018

@delner Added some rspec unit tests. There were already some tests for workers in test/workers_test.rb but decided to use rspec as the majority of the suite is there and it has an expectation for not raising errors (minitest doesn't :/).

Let me know if this is enough, or you'd prefer it any other way.

@delner
Copy link
Contributor

delner commented Jul 12, 2018

Yup, RSpec is what we prefer anyways, so that's all good! I'll take a look.

@delner delner added this to the 0.13.1 milestone Jul 13, 2018
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for your contribution! This will roll with 0.13.1.

@delner delner merged commit ad4cd16 into DataDog:master Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants