behaves icon indicating copy to clipboard operation
behaves copied to clipboard

Error doesn't raise until `exit` is called

Open edisonywh opened this issue 5 years ago • 6 comments

As pointed out by @PikachuEXE over at #12, errors won't actually get raised until exit is called. This is because of the use of at_exit, which only gets run... at exit.

Pikachu has proposed a new syntax, which uses block to mitigate this issue over at #12.

Pros:

  • Much easier to reason and understand when is the actual check ran
  • Mitigate a couple of other issues like #2 (which was temporarily fixed by testing private method).

Cons:

  • Drastically different usage, less "ruby-like", more opinionated (has to define methods inside block)

I'm not sure what's the best way to go about this here so opening this up for discussion, however Pikachu's proposal, despite changes the usage, has the cleanest strategy so far, so I'm leaning towards that.

Steps to reproduce

  • After cloning behaves, in the same folder you can do bin/console which brings up a console to play with.

  • Paste in this snippet of code:

class Animal
  extend Behaves

  implements :method_one, :method_two
end

class Dog
  extend Behaves

  behaves_like Animal
end
  • You'll see that the errors aren't raised until you type exit.

Anyone care to chime in? :)

edisonywh avatar Jun 18 '19 13:06 edisonywh

Can we do it this way, rather than raising an exception, should we just implement the methods in the Behaving class. The default implemented method returns nil, then if the client wish to override this method they can choose to define the method with their own definition. What do you guys think?

NavindrenBaskaran avatar Jun 18 '19 15:06 NavindrenBaskaran

Then you've just reimplemented inheritance's behaviors, which makes runtime errors a thing again (exactly what this gem is trying to prevent)

edisonywh avatar Jun 18 '19 16:06 edisonywh

I choose to raise on block exit since that's most DSL does things Since ruby classes are open ended so we don't have a way to execute checks at the "end of class" These hooks are also useless in this case:

  • included
  • extended
  • prepended
  • inherited
  • method_missing

Or you will have to enforce users to put a check statement at the end of implementation:

class Dog
  extend Behaves
  def die
     # arrrr
  end
  implements Animal
end

But this looks worse than block style for me :P

PikachuEXE avatar Jun 19 '19 03:06 PikachuEXE

Then you've just reimplemented inheritance's behaviors, which makes runtime errors a thing again (exactly what this gem is trying to prevent)

@edisonywh could explain how will it cause runtime errors? Because if a class behaves like another its methods should implemented by the Behaving class. In this case we want them to explicitly define the method, but my suggestion is like why can't we implicitly define it when we encounter this behaves_like: Animal in the Behaving class.

The newly implicitly implemented method for the Behaving object returns nil, is the same thing as them explicitly defining a method in the Behaving class which returns nil?

NavindrenBaskaran avatar Jun 19 '19 12:06 NavindrenBaskaran

Can't understand your point Would be great to see some code example

PikachuEXE avatar Jun 19 '19 13:06 PikachuEXE

Yep, agreed, maybe some code example would be great @NavindrenBaskaran, but what I meant is basically what is written in the README.

Let's take a look at inheritance-based behaviors:

Inheritance-based

class Animal
  def speak
    raise NotImplementedError, "Animals need to be able to speak!"
  end

  def eat
    raise NotImplementedError, "Animals need to be able to eat!"
  end
end

class Dog < Animal
  def speak
    "woof"
  end
end

With this approach, the following can happen on production:

corgi = Dog.new
corgi.eat # => NotImplementedError, "Animals need to be able to eat!"

Keep in mind that it does not matter if the parent class decides to return NotImplementedError or nil, either pattern is sufficient to indicate an overridable method (imo).

With Behaves

With behaves, supposedly you can't do this on production:

corgi = Dog.new
corgi.eat # => NotImplementedError, "Animals need to be able to eat!"

Why? Because you would've called behaves_like Animal inside Dog, and with that, you won't be able to have a dynamic error on production, because your code will fail when file is loaded and it won't be able to sneak to production.

What I gathered from the approach you're suggesting seems to be along the lines of injecting default behavior into behaving object (feel free to correct me), like this:

class Animal
  implements :speak, :eat

  inject_behaviors do
    def speak; nil; end
    def eat; nil; end
  end
end

class Dog
  behaves_like Animal
end

The reason that this does not make sense is because now Dog has not actually implemented the behavior, but rather have an injected version from Animal. This makes code like this possible on production:

bad_corgi = Dog.new
bad_corgi.eat # => NotImplementedError, "Animals need to be able to eat!"

Which has effectively reimplemented classes, and also rendering behaves itself obsolete.

Does that make sense?

edisonywh avatar Jun 20 '19 14:06 edisonywh