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 classes that extend modules where contracts were included #198

Conversation

waterlink
Copy link
Collaborator

Fixes #176

Our current routine while including or extending Contracts is to define Contract(*args) method for current class and make it just delegate to self.class.Contract. But in case of:

m = Module.new do
  include Contracts::Core
end

Class.new do
  include Contracts::Core
  extend m

  Contract String => nil
  def foo(x)
  end
end

it doesn't work, since there is no self.class.Contract at this point of time. But, there is just Contract already in super class, and we may just call it. That is what this PR does: verifies if there is a Contract method in super class and calls it instead of delegating to self.class.Contract.

/cc @PikachuEXE @egonSchiele

@waterlink
Copy link
Collaborator Author

@waterlink waterlink force-pushed the fix/176/classes_with_extended_modules_with_contracts branch from 53a069b to 3864490 Compare September 9, 2015 15:18
@waterlink
Copy link
Collaborator Author

Hmm, this fix doesn't work with ruby 1.9.2 only.

@waterlink
Copy link
Collaborator Author

Ok, now it works for every ruby :)

@PikachuEXE
Copy link
Collaborator

@waterlink
Your branch works for the test script and my project :)
Which is #198 ?

@PikachuEXE
Copy link
Collaborator

Wait I am in #198 now

@PikachuEXE
Copy link
Collaborator

I got another question
Should I include Contracts::Core or Contracts? (for latest versions)
I don't want to use the deprecated one :)

@alex-fedorov
Copy link
Collaborator

Contracts::Core is better in terms of not having all our helper classes in your namespace. But it means you don't have builtin contracts either.

Before we fully forbid usage of include Contracts, we will add include Contracts::Builtin. This way, you will be able to do:

include Contracts::Core
include Contracts::Builtin

And you will get original functionality that you have had before with include Contracts but without our helper classes in your namespace.

@egonSchiele
Copy link
Owner

lgtm

egonSchiele added a commit that referenced this pull request Sep 10, 2015
…modules_with_contracts

Fix classes that extend modules where contracts were included
@egonSchiele egonSchiele merged commit 0c068b1 into egonSchiele:master Sep 10, 2015
@PikachuEXE
Copy link
Collaborator

Hope to see Contracts::Builtin soon :)

@alex-fedorov
Copy link
Collaborator

@PikachuEXE this is very simple change. Maybe you are up for making a PR?

@PikachuEXE
Copy link
Collaborator

Contracts::Builtin or Contracts::BuiltinContracts?

@alex-fedorov
Copy link
Collaborator

Contracts::BuiltinContracts sounds like a repetition (you already have Contracts:: at beginning), so Contracts::Builtin it is.

@waterlink waterlink deleted the fix/176/classes_with_extended_modules_with_contracts branch September 15, 2015 02:24
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.

4 participants