contracts.ruby icon indicating copy to clipboard operation
contracts.ruby copied to clipboard

Do not require importing contract class names into client classes

Open robnormal opened this issue 9 years ago • 86 comments

On a large project, there may be a large number of contracts you will want to name, and these names will then be included in every class that includes the Contracts module. The potential for naming collisions could grow very quickly. (I am already having this problem - I have a class called "Maybe".)

One possible fix would be an alternative include, in which the Contract method takes a block instead of a hash.

class Alpha
  include ContractsSafe
  Contract { ArrayOf[Num] => Maybe[Or[This, That]] }
  # ...
end

The block could then be instance_eval-ed elsewhere, isolating the contract names.

robnormal avatar Jun 01 '15 10:06 robnormal

Related #101 and #105.

nixpulvis avatar Jun 01 '15 17:06 nixpulvis

This code is not a block now, but normal hash, probably you want something:

Contract { [ ArrayOf[Num] => Maybe[Or[This, That]] ] }

alex-fedorov avatar Jun 01 '15 18:06 alex-fedorov

Actually, it's a syntax error! The equivalent would be

Contract { { ArrayOf[Num] => Maybe[Or[This, That]] } }

which I have to admit is pretty ugly. Another possibility entirely - which would solve some other issues, including the load-order problem - would be to use a string to specify the contract. This could be eval-ed, or, better yet, parsed. I realize this is a pretty different direction than the project is currently going in, though.

robnormal avatar Jun 01 '15 19:06 robnormal

Hm, not really:

2.2.1 :001 > def Contract(*, &blk)
2.2.1 :002?>   end
 => :Contract 
2.2.1 :003 > Contract { [ String => String ] }
 => nil 

waterlink avatar Jun 01 '15 20:06 waterlink

You are wrong here, it is still possible to shape this project if it is worth it. We had long conversation on gitter some time ago about this and arrived at conclusion that we want to have block/eval/whatever there, because it allows to have more advanced type system.

waterlink avatar Jun 01 '15 20:06 waterlink

The trick is going to be moving forward with this while maintaining functionality.

nixpulvis avatar Jun 01 '15 20:06 nixpulvis

Yeah, of course =) Not really a big of a problem. And remember, we can always break an API, especially if we find some better API than current one (I doubt so, but everything is possible), if we provide nice deprecation-removal curve.

EDIT: and of course, don't forget, we are at 0.* version still, so that means that public API is still not frozen

waterlink avatar Jun 01 '15 20:06 waterlink

Supporting current API and block-based one, not really a big deal, since it is totally different calls and may even result in instantiating totally different decorator classes for instance.

waterlink avatar Jun 01 '15 20:06 waterlink

True, but I'd expect (might not be a huge deal to ensure this) Contract A ... => B === Contract { A ... => B } ignoring the syntax issues.

nixpulvis avatar Jun 01 '15 21:06 nixpulvis

How about Contract { A >= B } ? :) This is a valid ruby code, and I see how to implement it. Even so it looks strange

waterlink avatar Jun 01 '15 23:06 waterlink

Heh. Ended up doing some black magic:

Contract { A } >-> { B }

EDIT: btw, valid ruby: x >-> ... = x.>(-> ...)

waterlink avatar Jun 01 '15 23:06 waterlink

Allows to be more haskelly:

Contract { A } >-> { B } >-> { C } >-> ...

waterlink avatar Jun 01 '15 23:06 waterlink

Before I spend too much time thinking about what you're doing here. I'd be very cautious of the notion of currying here. Since Ruby is not implicitly curried.

nixpulvis avatar Jun 01 '15 23:06 nixpulvis

If you haven't yet, look at this stuff: https://github.com/txus/kleisli

waterlink avatar Jun 01 '15 23:06 waterlink

Very cool library, but I would avoid doing this here. Ruby is not Haskell and contracts are perfectly well defined in both contexts. It is true that Haskell lends itself much better to these kinds of ideas, but we should not depart to far from Ruby syntax.

This is such a perversion of Ruby syntax that Matz would probably condemn this

I agree.

nixpulvis avatar Jun 01 '15 23:06 nixpulvis

I know, I am not going to, it is just that the one who wants to find a way, will always find it.

Still, what about this one?

Contract { [A, B => C] }

waterlink avatar Jun 01 '15 23:06 waterlink

That's still probably one of the best options. Has anyone considered parsing our own contract language before? Is this something we are 100% opposed to?

nixpulvis avatar Jun 01 '15 23:06 nixpulvis

No, not 100% at least. But that would mean writing proper lexer and parser to get what we want. It would mean parsing free-form ruby + stuff we want to add on top. The only way is to cut features to end up with something manageable.

For example, this probably makes sense to be cut: Contract "lambda { |x| x.age > 12 }" - because it is something that can have free form ruby inside.

waterlink avatar Jun 01 '15 23:06 waterlink

Or we might just do eval-ing to ruby: Contract Contracts::LateBinding[MyContracts, "A, B => C"] will evaluate the string once, but in different context.

waterlink avatar Jun 01 '15 23:06 waterlink

This might create difficulties for tracing errors, when you have typo in that string..

waterlink avatar Jun 01 '15 23:06 waterlink

Other thing, that is dangling in my mind is to allow very verbose syntax like that:

Contract do
  inside(MyContracts) do
    argument A
    argument B
    keyword_argument D, default: D.default_value
    result C
  end
end

waterlink avatar Jun 01 '15 23:06 waterlink

The beauty of parsing our own language would be the ability to avoid all the namespacing and syntax restrictions.

To @robnormal's point we would be smart to evaluate contracts within a namespace with the contract classes defined inside, to avoid global pollution.

In terms of errors yes a potential point for errors, but a smart parser could handle this. I've used kschiess/parslet a little bit before, with great results.

Regardless of parsing or not, I think the main point of this issue should not be an opt-in thing. It should be default behavior.

nixpulvis avatar Jun 01 '15 23:06 nixpulvis

By default, you mean this: include Contracts will add only Contract method and nothing else. Everything else is up to user. ?

waterlink avatar Jun 01 '15 23:06 waterlink

Well, and that in addition to only adding the Contract method, all contracts are evaluated in their own namespace. This would be exceptionally helpful for defining "types" (classes with a valid? method) for other classes with the same name. I've personally wanted this a number of times.

nixpulvis avatar Jun 02 '15 00:06 nixpulvis

Can you provide small example? I can't understand what do you mean by "all contracts are evaluated in their own namespace".

waterlink avatar Jun 02 '15 00:06 waterlink

Do I understand it right, something like that:

Contract(App::Engine::Contracts, Contracts::Builtin) { [A, B => Not[C]] }

EDIT: this might be noisy to my taste, don't know.

waterlink avatar Jun 02 '15 00:06 waterlink

class A
  # Bunch of application logic.
end

define_contracts do
  class A
    def valid?
      # ...
    end
  end
end

class B
  Contract "A => Num"  # using a string to show that this isn't necessarily correct syntax.
  def foo(a)
    # ...
  end
end

This way there are two As, one for the actual class with application logic, and one for the type of A in the case where I want to do more than the default logic for a class.

define_contracts could especially be a way to add constants to another namespace. This way the global namespace is not cluttered with "types".

UPDATE: Contract form changed.

nixpulvis avatar Jun 02 '15 00:06 nixpulvis

Don't really see, how it will work without a block or a string/eval. The closest will be Contract { [A => Num] } again..

waterlink avatar Jun 02 '15 00:06 waterlink

This is nice too because it would still work if users defined constants on the global namespace, however it provides a way to avoid that.

Oh and, I should have used some other notation for the contract, yes. We'll need to figure out the block contract form for this. Updated above.

nixpulvis avatar Jun 02 '15 00:06 nixpulvis

Now I get with that example. So all contract definitions will be in one namespace isolated from user codebase. Not bad idea.

waterlink avatar Jun 02 '15 00:06 waterlink

@egonSchiele @sfcgeorge @robnormal What do you think about the last idea from @nixpulvis?

alex-fedorov avatar Jun 02 '15 11:06 alex-fedorov

The idea is kinda nice, isolating namespace, being able to have a contract with the same name as the class you're validating, etc.

I don't like using a string at all. Another project already does that anyway. I like Contracts looking like part of Ruby and being syntax highlighted etc.

sfcgeorge avatar Jun 02 '15 11:06 sfcgeorge

Then { [A, B => C] } ? Or anything else you can see?

alex-fedorov avatar Jun 02 '15 11:06 alex-fedorov

Yeah I can't see any way of making a Block without curly brackets. So then either a hash or array inside that.

Contract { [A, B => C] }

Contract {{ A, B => C }} # I slightly prefer this, quicker to type too

sfcgeorge avatar Jun 02 '15 11:06 sfcgeorge

I also prefer {{ X => Y }} to {[ X => Y ]}.

The only downside of @nixpulvis's idea is, you would have to write a contract class for every class you wanted to type-check for. I suppose we could use constant_missing to default to the current behavior.

robnormal avatar Jun 02 '15 19:06 robnormal

Not necessarily. We could make it so it simply executes within a namespace of the current context with the base contracts included. That way the classes and variables where a contract is attached are available.

nixpulvis avatar Jun 02 '15 20:06 nixpulvis

I like define_contracts too, seems like an easy way to put Contracts in a hidden namespace. I don't like the idea of eval-ing strings though.

egonSchiele avatar Jun 03 '15 00:06 egonSchiele

I like this. But: the question is, will this approach with hidden namespace and contracts with the same name create any confusion for readers of the code? What about autoloading that is done by certain libraries out there?

For example in my code, if I want to have my own custom contract (implements .valid?), but I want still to express that this argument should be of certain class I just append Contract suffix for the name of the contract, ie:

class Person
end

class A
  include Contracts

  Contract NamedPersonContract => String
  def fancy_name_for(person)
    # ...
  end
end

NamedPersonContract = Contracts::RespondTo[:first_name, :last_name, :nickname]

Or other thing I considered using in this case: Contract Person::NamedContract => String

waterlink avatar Jun 09 '15 23:06 waterlink

Adding Contract to the end of every contract seems overly verbose and likely to not be done, causing consistency problems. The ability to define a contract with any name (even one used in the application logic) and not collide seems positive, but there shouldn't be anything stopping you from not putting it in the special namespace, and avoiding collisions on your own as well.

nixpulvis avatar Jun 09 '15 23:06 nixpulvis

I would prefer parsing the string instead of doing evaluation because of the possible security problems.. Also consider this code goes to production and it's only controlled by a environment variable which can be easily manipulated by an attacker... Also, it's never encouraged to use eval in almost every language..

Parsing is a much better option but we also should keep in mind it has to NOT slow down too much.. e.g. parsing can be done when evaluating class code and not every time the method is called, etc...

alem0lars avatar Jun 10 '15 06:06 alem0lars

So.

I want to see something like include Contracts::Core in the wild, that will not pollute current namespace with anything at all. No builtin contracts, nothing. include Contracts will still work as it worked, because we probably don't want to break people' code.

Examples:

# old way, with polluting of namespace:
class X
  include Contracts

  Contract Num, Maybe[Num] => Float
  def fetch(a, b)
    # ...
  end
end

# new way, without namespace polluting:
class X
  include Contracts::Core

  Contract Contracts::Num, Contracts::Maybe[Contracts::Num] => Float
  def fetch(a, b)
    # ...
  end
end

# block form, shorter, don't need to spell out `Contracts::` for builtin contracts:
class X
  include Contracts::Core

  Contract {{ Num, Maybe[Num] => Float }}
  def fetch(a, b)
    # ...
  end
end

At some point, probably 1.0.0 release, we can make include Contracts behave like include Contracts::Core as described above. And if we want to leave old polluting behavior at all, then something like include Contracts::Full or something.

This is to stop polluting people' namespaces and deal with name clashes with different gems. Definition of your own contracts in separate hidden namespace is probably next feature after that.

WDYT?

waterlink avatar Jun 10 '15 22:06 waterlink

Seems like a classic deprecation issue. We should defiantly deprecate the include Contracts behavior. I'd like to allow for that to stay be the main method of including, without a need for another include Contracts::Core. I think we should also deprecate non blocked contracts, since many features could make good use of contracts being blocks. Before we can say if this is a good or bad idea though, we'll want to have some benchmarks on performance, and strategies for pre-evaluating the blocks.

Overall I'm not a fan of include Contracts::Core, but I do think we should add it in the mean time while we deprecate the functionality of include Contracts. Then at some point remove it.

nixpulvis avatar Jun 10 '15 22:06 nixpulvis

Thinking more about it, I don't see any way we can defeat the problem of the lexical scoping of constants here, except to use strings for the contracts; if we want to avoid class load-order issues, that is.

robnormal avatar Jun 10 '15 23:06 robnormal

No strings please, there are several other similar libraries that use parsed strings so there's no point stepping on their toes. There's currently choice which is good, Contracts using native ruby syntax, or the others using parsed strings. Use whichever you prefer :)

I like @waterlink's idea above. Version 1.0 removing old behaviour and swapping Contracts::Core for Contracts seems like a good upgrade path. I'm not sure why you'd want to use the verbose syntax, I'd ultimately be in favour of just having the double brace syntax and nothing else if it makes Contracts' internal code simpler.

I don't think making scoping work is a problem, instance_eval should do it as Ruby lets you leave off namespaces you're already inside.

sfcgeorge avatar Jun 11 '15 00:06 sfcgeorge

I'm afraid instance_eval is no help here. Constants are lexically scoped, meaning the Alpha in Contract { Alpha } is bound to the class where this statement occurs, not the class that evaluates the block. I don't think Ruby has a facility for circumventing this.

robnormal avatar Jun 11 '15 00:06 robnormal

Well you see, it is possible. Though I'm not 100% sure we want to be doing it.

def do_it(&block)
  new_block = proc do |s|
    old = s::Foo.send(:remove_const, 'VAL')
    s::Foo.const_set('VAL', 1)
    block.call(s)
    s::Foo.send(:remove_const, 'VAL')
    s::Foo.const_set('VAL', old)
  end
  class_eval(&new_block)
end

module A
  class Foo
    VAL = 1
  end
end

class B
  class Foo
    VAL = 2
  end

  do_it { puts Foo::VAL }
  puts Foo::VAL
end

Just a proof of concept.

nixpulvis avatar Jun 11 '15 01:06 nixpulvis

I've seen something that crazy with changing the binding of a proc but it only works for variables and methods—not constants—because you can't really change the binding, only the value of self to proxy method calls (which variables act like but constants don't).

const_missing is another dead end as in the case of a naming collision it obviously won't be missing.

@robnormal yeah sorry I was wrong. Forgot that instance_eval changes self which is only good for proxying method calls and variables, not constants which are lexically scoped as you say. Here's a method of proxying variables and method calls (not constants) that's interesting nonetheless: http://stackoverflow.com/a/10059209

Nice @nixpulvis . I'm ok with magic (especially if it's optional) but I think @egonSchiele isn't.

sfcgeorge avatar Jun 11 '15 02:06 sfcgeorge

It's either magic, strings, or explicit namespaces. Choose one.

nixpulvis avatar Jun 11 '15 02:06 nixpulvis

Another huge point against the code I posted is thread safety. Which unless Ruby does something fancy I don't know about, will defiantly be broken in a horrific way by this.

I posted more or less just as a means of showing the only way I could think to do it.

nixpulvis avatar Jun 11 '15 02:06 nixpulvis

I vote for doing a DSL with it's own parser. It's the most clean way to accomplish this without dirty hacks and being thread safe. Also, it gives us full control over both syntax and semantics, which is great for the future..

alem0lars avatar Jun 11 '15 15:06 alem0lars

I'm always a fan of parsing things. So this might be my suggestion too. Stands to solve a lot of problems.

nixpulvis avatar Jun 11 '15 15:06 nixpulvis

Another option would be to use the block syntax, lower-case names for contract methods, and symbols for class and module names. This would solve both the naming conflicts and the scope / load-order issues.

robnormal avatar Jun 12 '15 02:06 robnormal

Played around in irb.

1st possibility:

Contract {{ Num(), Maybe(Num()) => String }}

2nd:

Contract {{ c.Num, Maybe(c.Num) => String }}

3rd:

Contract {{ num, maybe[num] => String }}

All of them are methods, not constants. And when you define contract with Contracts.define_contract(:AContractName) { .. class body here .. } this method gets defined on special hidden module, that gets instance_evaled with provided block. Actually it can support all 3 of them (by defining aliases of name in camelcase and snakecase; and by defining method c which just returns self).

rough implementation:

module Contracts
  def self.define_contract(name, base=BaseContract, &blk)
    BaseContract.build(name, base, &blk)
  end

  class BaseContract
    def self.name
      @name || super
    end
    # + :{self.,}to_s, :{self.,}inspect

    def self.build(name, base=self, &blk)
      contract = Class.new(base) do
        @name = name
      end
      contract.class_eval(&blk)

      aliases.each do |name|
        HiddenNamespace.module_eval do
          define_method(name) do
             contract
          end
        end
      end
    end

    def self.aliases
      [name, Support.camelcased(name), Support.snakecased(name)].uniq
    end
  end

  module HiddenInterface
  end

  def Contract(&blk)
    raw_contract = HiddenInterface.instance_eval(&blk)
    Contract.new(raw_contract)
  end
end

waterlink avatar Jun 12 '15 11:06 waterlink

The problem with using a symbol to look up constants is that then you can't use a symbol as a contract, and it's useful being able to use any Ruby value as a contract.

I still don't like parsing a string as it won't be syntax highlighted so doesn't look as natural, and we already support so many features it would be a massive undertaking doing it—we'd have to use a full ruby parser library and from experience they suck (Ripper). Or just eval the string, which is evil. I think it would likely cause more trouble than it solves.

I've messed around and searched a lot and it seems sadly it's really not possible to change a proc's binding or lookup. I've wanted that ability several times, alas no. It was possible in 1.9.1 but caused back compat problems.


@waterlink I love your first option! It's a few more characters than our current/old way but still in the spirit of Contracts. Many votes for that.

My only issue with 1 is that String doesn't have the parens, it's obvious why to us, but to a beginner I think they'll assume that when writing a contract every argument must have parens and be confused when String() isn't a method. I suggest adding a method_missing that looks up a constant with that method name and uses it if it exists. Is that too magic? Alternatively we could have shorthand contracts for all common built in types so String == Str().

Option 2 is alright I guess, I can't think of a use case for it though.

Option 3 I'm not sure about; people might want to define a method or variable as a shortcut to a longer contract and we could accidentally clobber that. Arguably you should define a custom contract in that case, but it's nice having the option.

stringy = Or[String, Symbol]
=> #<Contracts::Or:0x007f8fda401200 @vals=[String, Symbol]>

Contract stringy => Any
def polite(name)
  name.to_s.capitalize
end

=> :polite
polite "joe"
=> "Joe"
polite :joe
=> "Joe"

sfcgeorge avatar Jun 12 '15 11:06 sfcgeorge

I don't think syntax highlighting is a good reason because :

  1. it can be easily added (it's very simple in vim, emacs, intellij idea, sublime text, atom, etc..)
  2. this is an infrastructural thing.. I wouldn't design a software based on tools but tools based on software..
  3. Using an operational semantic to emulate another is the source of the problem and cannot be solved otherwise
  4. making a poor design decision based on "the colors we see in the editor" without any other advantage will lead to problems in the future. Plus, actually you see wrong colors when using Contract (...)
  5. Parsing our dsl allows us to GAIN A REALLY HUGE PERFORMANCE BOOST.. If you are already using this gem you will notice performance bottlenecks.. Parsing leads us to cook the behavior at parsing time, not every time a method is called.
  6. Parsing allows us to prevent weird behaviour / dependency cycles
  7. You don't know the future, parsing is an OPEN solution, using ruby hacks is a closed solution which can prevent u some beautiful stuff
  8. Parsing leads to a more maintainable solution.. Messing with ruby internals / abusing metaprogramming makes code really unmaintainable

alem0lars avatar Jun 12 '15 12:06 alem0lars

@waterlink I find the third syntax to be the most readable by far, as well as the most transparent to a Ruby programmer. @sfcgeorge Not sure where you see a conflict. Just as users could not define a "Maybe" contract without overwriting the existing one, they could not define a "maybe" method without overwriting the existing one. This is standard, no?

Incidentally, I'd like to point out that eval is not "evil"; calling eval on end-user input is a security hole, that is all. If you're putting end-user input in your contract definitions, you are most definitely doing it wrong.

robnormal avatar Jun 12 '15 12:06 robnormal

@alem0lars I don't really see any performance boost from parsing a string.. The only bottleneck contracts currently has is Contract#call_with which only handles actual arguments passed to method. Everything else is already built and memoized properly (validators); it just calls validators on each argument. And string there will make it look not as pretty as it looks now (at least for me).

About metaprogramming: Can you read the codebase and point me at any ruby hacks in code base? Because currently it is very simple and non-intrusive as possible.


@sfcgeorge I have came up with 2nd and 3rd syntax, because I personally don't like these () there all over the place, and especially mixing having them and not having them in different cases. Latter can be partly fixed for core ruby classes, but not for user classes, which can be namespaced and we actually want there normal constant lookup.

waterlink avatar Jun 12 '15 12:06 waterlink

BTW, I have just noticed, that {{ a, b, c => d }} is not working :) So it leaves only {[ a, b, c => d ]}

waterlink avatar Jun 12 '15 12:06 waterlink

@sfcgeorge in 3rd example there will not be a conflict:

stringy = Contracts::Or[String, Symbol]
Contract {[ stringy => any ]}
# => [{ Contracts::Or[String, Symbol] => Contracts::Any }]

So it will use user's stuff by default. And that is why you would need c.:

stringy = Contracts::Or[String, Symbol]
Contract {[ c.stringy => any ]}
# => [{ Contracts::UserContracts::Stringy => Contracts::Any}]

In this case c. just gives user a control.

waterlink avatar Jun 12 '15 12:06 waterlink

There is totally different possibility:

Contract Contracts[:Num], Contracts[:Maybe][Contracts[:Num]] => String

Which is very verbose, but no magic involved at all.

waterlink avatar Jun 12 '15 12:06 waterlink

About {{ .. }} vs {[ .. ]}: I was thinking about something a bit more verbose, but less problems:

Contract { self[num, maybe[num] => String] }
# or
Contract { self.(num, maybe[num] => String) }
# or even
Contract { self.<< num, maybe[num] => String }
# and of course for brevity
Contract { c.<< num, maybe[num] => String }

waterlink avatar Jun 12 '15 12:06 waterlink

And continuing the idea of .<<, I just thought about nice distinguishing between input and output contract:

Contract { c.<<(num, maybe[num]).>>(String) }
# or
Contract { c.<<(num, maybe[num]) >> String }
  • .<<(...) - input
  • .>>(...) - output

waterlink avatar Jun 12 '15 12:06 waterlink

@waterlink It's not that you parse the string. It's that you can do it ahead in time and don't couple contract checking to the contract structure. With Ruby hacks you need to couple the evaluation to the structure you want to give to DSL.

alem0lars avatar Jun 12 '15 13:06 alem0lars

@alem0lars Yep speed problem is almost entirely in call_with which we've rewritten a few times and it's tough to make any faster. It's actually calling the method that is slow, not validating the contract. We'd need a Proc.to_ruby to make it faster by using the raw source. Other similar libraries are faster because they don't have as many features (method overloading), or they use a Ruby C extension. We'll have to agree to disagree on the importance of the way it looks.

Here are alternatives for inspiration: https://github.com/janlelis/sig (simpler) https://github.com/gogotanaka/Rubype (faster) https://github.com/plum-umd/rtc (string parsing)

@waterlink thank you for all your explanations. Ok the 3rd way is growing on me and I understand the use of the 2nd way. This approach seems very flexible, any combination can be used to suit needs.

Ah yes I see the problem with {{ .. }}. But {[ ... ]} is similar so I don't mind it :)

I'd rather stick with {[ ... ]} than the more verbose options. The .<< etc ideas are interesting but starting to look not very rubyish. It could be made slightly shorter but still odd?

Contract { _(num, maybe(num)) >> (String) }

Ok more devil's advocate, would there be clashes with c. if a user defined a method called c? Should there be a way to change the default letter, or pass a variable into the block?

Contract { |x| { x.num, maybe(x.num) => String } }

sfcgeorge avatar Jun 12 '15 13:06 sfcgeorge

I personally do not like the .>> or .<< methods. We should aim to keep the syntax as close to what you'd expect from a type declaration.

nixpulvis avatar Jun 12 '15 18:06 nixpulvis

I like @waterlink's idea of include Contracts::Core. Maybe we could create an alias for the contracts class, so you can write a contract like this:

Contract C::Num, C::Num => C::Num

This is a nice discussion! I just want to make sure that we don't make the syntax too magical...if it moves too far from regular ruby I think it will turn off users.

Also I don't think we need to use a chainsaw where a pair of scissors would work :) Writing our own parser feels pretty heavyweight to me.

egonSchiele avatar Jun 13 '15 01:06 egonSchiele

Agreed, as much as I'd love to write a parser. The more I think about it I'm starting to lean heavily to using Contract Contracts::Num => Contracts::Num and having a way to by default bind C to Contracts for convenience.

Only thing to consider then is not binding to C if the user has that constant defined, which is there responsibility.

nixpulvis avatar Jun 13 '15 01:06 nixpulvis

It sounds like we're agreed that all contracts should be defined under the Contracts module. So the least magic way of writing contracts would be:

Contract Contracts::Num => Contracts::Num

Now, for anyone who isn't having naming collisions with the built in contracts, it is easy to get back the current really nice short syntax by just including the namespace:

include Contracts::Contracts

Contract Num, Maybe[Num] => String

Then there could be 2 more concise ways of accessing them, depending on preference, both avoiding name collisions.

The first as @egonSchiele suggests is the simplest. The advantage is no weird syntax and easy implementation to add. I suggest having it as an option going forwards regardless of whether we also add the more advanced option. I propose changing it a little to allow the user to choose the letter shorthand to avoid collisions:

include Contracts::Core
register_contracts(:c)

Contract c::Num, c::Num => c::Num

Finally there's the brace-box style, which has the advantage that it will allow the addition of more advanced features like Haskell-esque stuff that was discussed before. I like it better as it looks nicer and requires less typing (just 4 more characters than the current way). @waterlink 's example implementation looks nice.

Contract {[ num, maybe[num] => String ]}

So I'd quite like to see both. The simple way because it's little effort to add, and the advanced way because it allows for more features and a slightly terser syntax in some cases.

sfcgeorge avatar Jun 13 '15 16:06 sfcgeorge

@sfcgeorge :+1: for register_contracts, but I would:

  1. rename it to register_contracts_shortcut - it reveals intent better
  2. don't pollute user class with this method, ie:
class Greeting
  include Contracts::Core
  Contracts.register_shortcut(self, :c)
end

This way, it will not create an additional one-time-use method on user's eigenclass, but still allow to do the same thing. And additionally it is possible to do this for all classes/modules at once if one need it: Contracts.register_shortcut(Module, :c).

waterlink avatar Jun 13 '15 16:06 waterlink

@waterlink You're right. Perhaps Contracts.alias(:c) to be consistent with the ruby alias_method. I'm also wondering if that method could do the include Contracts::Core bit to save a line in user code.

sfcgeorge avatar Jun 13 '15 16:06 sfcgeorge

Yes, it obviously could, because include is just usual ruby method defined on all modules/classes, that you can just send to:

def alias(target, name)
  define_shortcut(target, name)
  target.send(:include, Contracts::Core)
end

waterlink avatar Jun 13 '15 16:06 waterlink

But in that case, alias method would be strange.. How about:

class Greeter
  Contracts.activate self, core: true, alias: :c
end

?

waterlink avatar Jun 13 '15 17:06 waterlink

But this makes it kinda unusual and not that simple. And people probably still would want to use normal include Contracts (+ ::Core) better than that.

waterlink avatar Jun 13 '15 17:06 waterlink

Why not overload the Contracts callable, so that it can take 1+ arguments, in the Contracts::Xxx style, or 0 arguments and a block, in the {[ ]} style?

robnormal avatar Jun 13 '15 17:06 robnormal

@robnormal That is exactly what we want to do. Actually, overload Contract constructor. What we are trying to discuss here additionally - is a way to provide optional shortcut feature. Effectively making Contracts::Or => c::Or (or c.Or), where c is specified by user.

waterlink avatar Jun 13 '15 17:06 waterlink

Actually, I don't see any problems to implement this without shortcut functionality and add shortcut functionality later (think: agile, mvp, etc).

Rough plan:

  1. 1st PR: Introduce include Contracts::Core, add it to docs.
  2. 2nd PR: provide shortcut option (if we agree we need one).
  3. Release 0.10.
  4. Update 0.99 branch from current master.
  5. 3rd PR to 0.99: add deprecation warning on include Contracts, update docs.
  6. Merge and Release 0.99.
  7. 4th PR: break include Contracts with raising helpful error with link to documentation.
  8. ... Whatever else we would like to break ...
  9. Release 1.0.0 (I would like to have proper semver starting from first major release).

WDYT?

waterlink avatar Jun 13 '15 18:06 waterlink

Maybe I'm just not following well, but I don't really like the idea of using include Contract::Core as the primary interface. I'd rather deprecate using things like Num without a namespace.

Also maybe I'm missing something but why do we need a method for alias what about this?

Include Contracts
C = Contracts

Would this be enough?

nixpulvis avatar Jun 13 '15 22:06 nixpulvis

The problem with include Contracts is that it will include the whole namespace unless you re-define append_features. But if you do so, and for example include contracts in a module, then you will re-define this user's module append_features too, which will break their code and make a lot of confusion and frustration. It can be worked around, but it sounds like source of bugs.

So it is definitely some isolated module that has almost nothing inside, like: Contracts::Core or Contracts::Base or Contracts::Helper.

Why do you dislike include Contracts::Core? You need only one per class and it doesn't really change much from user's perspective, but it does make nameclashes go away for sure.

C = Contracts enough and works. Though I doubt most of people will use it, even if we document it somehow. But if we provide them with nice public api method to do that kind of shortcut, and use it throughout all the examples, then they will probably use it more.

Why would we want most of our users to actually use this shortcut? - just because it is not fun to type Contracts:: each time you want to refer builtin contract. With Contracts:: all over the place, your contracts definitions, even for short methods (1-2 args) can exceed line limits of your team coding style/your editor/whatever. Which is frustrating and may result in people dropping contracts at all or prefer different library.

Still, I want to see not only C::, but even shorter version c. (or whatever letter/short word user wants) - it is easier to type (no constant shift pressing).

You may think, that it is not constructive to optimize for typing.. But we do want to provide concise and fun way of defining contracts and stop nameclashing with different libraries and user code.

Can you think of another way to achieve both?

waterlink avatar Jun 13 '15 22:06 waterlink

Ok I see your point about the submodule. In terms of the aliasing, I'm starting to like the idea of not using include as the primary way of pulling contracts into a module. Something we have more control over like activate is more flexible.

nixpulvis avatar Jun 13 '15 22:06 nixpulvis

@egonSchiele What is your take on this? If you agree with last ideas, I can send a PR(s) for them.

alex-fedorov avatar Jul 02 '15 09:07 alex-fedorov

Just read the thread now.

I like Contracts.activate as a way of making a class/module contracts-enabled. It means you can grow the interface over time if needed.

I think users should be able to enable it at the top level, too. Why should every class need it? When you choose to use contracts, you pretty much decide to use it throughout your code. I'm not familiar with the issues, though. If the top level is a special case, then maybe Contracts.activate_top_level or something...

I'm not at all keen on anything other than Contract Num, String => Bool. All the ideas above are complex and involve unwanted extra typing. It doesn't take much of that before a user will say "can't be bothered".

gsinclair avatar Jul 04 '15 14:07 gsinclair

@gsinclair I agree with liking the existing really nice short syntax. Actually with the proposed change, you could still use that exact syntax, just the include statement would change slightly. I've updated my summary comment to include an example of that. https://github.com/egonSchiele/contracts.ruby/issues/157#issuecomment-111726998

To include contracts in every class you could just re-open Object and stick them in there, then you always have contracts. This works in the current version and should continue working.

class Object
  include Contracts
end

sfcgeorge avatar Jul 04 '15 15:07 sfcgeorge

Doing 4th of July stuff here...will read over this after the weekend

egonSchiele avatar Jul 04 '15 16:07 egonSchiele

Sorry for my late response! I like the roadmap laid out by @waterlink here: https://github.com/egonSchiele/contracts.ruby/issues/157#issuecomment-111739067

include Contracts::Core vs Contracts.activate: we will be adding some extra cognitive overhead with .activate since include is more commonly used. Is there a benefit to .activate that makes the overhead worthwhile?

I'm thinking the same thing for C = Contracts vs Contracts.register_shortcut(:c). We used aliases like C = Contracts all the time at work, so that seems normal and easy to me. Is there a benefit to register_shortcut that we can't get otherwise?

egonSchiele avatar Jul 16 '15 15:07 egonSchiele

I'm totally up for include Contracts::Core and C = Contracts.

About C = Contracts it would be nice to have it as a documented example on how to avoid typing Contracts:: each time.

alex-fedorov avatar Jul 16 '15 16:07 alex-fedorov

:+1:

nixpulvis avatar Jul 18 '15 00:07 nixpulvis