rubiconmd / injectable

Opinionated and declarative Dependency Injection library for ruby.
https://rubygems.org/gems/injectable
MIT License
34 stars 5 forks source link

Ruby 2.7.1 warnings: AHORA SI QUE SI, CABESA #24

Closed julitrows closed 3 years ago

julitrows commented 3 years ago

This PR addresses the final warnings we were getting related to Ruby 2.7 change in argument delegation.

As it is, all specs are green in both Ruby 2.6 and 2.7.1

Suggestions to beautify the split_args and build_instance are welcome

iovis commented 3 years ago

Not sure about this, I would like to add more test cases.

Yeah, please add/suggest as many test cases as necessary, this is a first draft to have a starting point

iovis commented 3 years ago

I switched to Draft till we iron out any possible edge cases

rewritten commented 3 years ago

@jantequera @iovis now it covers every coverable case I can come up with.

Of course if the user passes a symbolic hash as the last positional argument, and the dependency has explicit kwarg initializer incompatible with the passed hash, it will explode, but I don't think it should be covered.

This is an example of the failing case:

class MyDependency
  def initialize(positional = nil, kw1: nil); end
end

class MyInjectable
  include Injectable

  dependency :my_dependency, with: [{foo: :bar}]
end

:foo will be passed as kwarg to the initializer, and it will raise an exception. Tell me if you want to cover also this case, because it will make the code a lot more complex, as I will have to introspect the signature of the initializer.

iovis commented 3 years ago

Seems like there are still some warnings for ruby 2.7 Screen Shot 2021-03-15 at 12 56 33

rewritten commented 3 years ago

Seems like there are still some warnings for ruby 2.7

@iovis These are unrelated to ruby 2.7, it complains for a constant defined in a block, and we should actually refactor the code to have those constants defined only once.

julitrows commented 3 years ago

@iovis where did you get those? Never saw them running the specs locally 🤔

iovis commented 3 years ago

In the PR checks for the ruby 2.7 build

codeclimate[bot] commented 3 years ago

Code Climate has analyzed commit 0b944ab7 and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 7
Style 2

View more on Code Climate.

iovis commented 3 years ago

Of course if the user passes a symbolic hash as the last positional argument, and the dependency has explicit kwarg initializer incompatible with the passed hash, it will explode, but I don't think it should be covered.

We can revisit if we run into this