Skip to content

Conversation

@BuonOmo
Copy link
Contributor

@BuonOmo BuonOmo commented May 10, 2021

Some popular gem redefine caller_locations, hence if used in
that context, dead_end would throw an error.

Reproduction:

require "sinatra/base"
require "dead_end"

class App < Sinatra::Base
  require_relative "some"
end

Thanks for the project, first time contributing hence please tell me if I have done something the wrong way.

I haven't added tests, didn't know in which file I should put those. I'll do if you tell me!

[Some popular gem][1] redefine `caller_locations`, hence if used in
that context, dead_end would throw an error.

Reproduction:

```ruby
require "sinatra/base"
require "dead_end"

class App < Sinatra::Base
  require_relative "some"
end
```

[1]: https://github.com/sinatra/sinatra/blob/5513eb3c7969dc3d15b3c704df63a3ccce9ac9cb/lib/sinatra/base.rb#L1553
@schneems
Copy link
Collaborator

Thanks for the PR!

Thanks for the project, first time contributing hence please tell me if I have done something the wrong way.

In this case it's pretty hard to write a test without having side effects on other tests (for instance if a test re-defined caller_locations method). So I think having the existing tests is enough to ensure that at least this code didn't break anything. Adding a test would ensure that we don't have an accidental regression, but I think that's not such a large deal since in this case it's a single line feature/change. In short: I think we're good to go!

If this change were larger then I would usually ask for some kind of a manually exercisable case. Basically, some kind of an Example App https://www.codetriage.com/example_app that showed the problem. That way I could try it with and without your patch to verify that it does what it says it does (i.e. did this actually fix the problem). As is I'm happy to merge. Thanks for the help!

@schneems schneems merged commit ac8cee8 into ruby:main May 10, 2021
@schneems
Copy link
Collaborator

v1.1.7 is released with your change

@BuonOmo
Copy link
Contributor Author

BuonOmo commented May 11, 2021

@schneems nice, thanks for the reactivity ! On the tests, I get you, thanks for the detailed answer. Being a SO user, I'm used to providing examples as well (in fact the reproduction in my commit message is what used to be called an MCVE).

Anyway, thanks, and I guess we might talk next time on derailed benchmarks 😄

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.

2 participants