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 for Sinject::DependencyContractInvalidParametersException #35

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

cosimaradu
Copy link
Member

@cosimaradu cosimaradu commented Mar 18, 2024

fix for Sinject::DependencyContractInvalidParametersException: The method signature of method: 'get' does not match the contract parameters: 'block'

Copy link

codeclimate bot commented Mar 18, 2024

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

The test coverage on the diff in this pull request is 100.0% (95% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

@cosimaradu cosimaradu self-assigned this Mar 18, 2024
@cosimaradu cosimaradu added the bug label Mar 18, 2024
@cosimaradu cosimaradu changed the title add rest fix for Sinject::DependencyContractInvalidParametersException Mar 18, 2024
@cosimaradu cosimaradu merged commit 3293eb4 into master Mar 19, 2024
12 checks passed
@cosimaradu cosimaradu deleted the ruby3_new_param_type branch March 19, 2024 14:57
@KevinBrowne
Copy link
Member

KevinBrowne commented Apr 9, 2024

What was this supposed to address? The description does not contain enough detail to understand that.

In any case, I believe that this change introduces a bug. If a contract class specifies keyrest args, and the concrete class doesn't, then the concrete class is not implementing the contract.

This contract states that #foo must accept any number of keyword arguments:

class Contract
  def foo(bar, **kwargs); end
end

This class does not satisfy the contract because it does not accept any number of keyword arguments, it accepts precisely one:

class Concrete
  def foo(bar, kwargs: nil); end
end

However, with this change the contract validation will now accept the class!

On the other hand, it will not accept this one because kwargs is keyreq:

class Concrete
  def foo(bar, kwargs:); end
end

I'm raising a bug for this, but if you're now depending on this behaviour, it's going to be difficult to fix it.

@cosimaradu cosimaradu mentioned this pull request Apr 11, 2024
KevinBrowne pushed a commit that referenced this pull request Apr 12, 2024
Revert changes made in #35.

Fixes #36. A key arg will no longer satisfy a contract specifying keyrest args.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants