Promised keys can be nil about light-service HOT 10 CLOSED

I agree with the notion of "nils are evil" and avoid it wherever you can. As I recall, when we set a promised key to nil, it had the sudtle meaning of being "optional" key-value in the context.

Would you require the value to be not nil with the associated promised key?

from light-service.

I would require the value of a promised key to be not nil as a matter of preference, since it was promised. Using the principle of least surprise, I would expect that promised keys are assigned with something that has value i.e. non-nil values. If it's optional, then it shouldn't be promised.

Now I've thought about the ways to get around this:

1. edit method_missing and don't use promises when you don't mean to promise it. I've seen parts of our own code when something promised was only there because of ctx.method convenience.

class Action
  include LightService::Action
  executed do |ctx|
    ctx.something = nil # ctx.something can be called without declaring as part of promised keys
  end
end

1. set something to be optional instead of promised. The advantage of this over number 1 is that you don't have to give up raising a NoMethodError when it's actually valid.

class Action
  include LightService::Action
  optional :something
  executed do |ctx|
    ctx.something = nil 
  end
end

from light-service.

I would stay away from using method_missing if I can.
We might have toyed with the idea of optional macro, but never really had the need to implement it.

What happens when an operation will return nil and that value is assigned to a promised key?

Like this:

class Action
  include LightService::Action
  promised :something
  executed do |ctx|
    ctx.something = SomeOperation.call # this will return a nil.
  end
end

If we made the promised key to "promise" non-nil value, we might have to do an extra check:

class Action
  include LightService::Action
  promised :something
  executed do |ctx|
    result = SomeOperation.call
    unless result.nil?
      ctx.something = result
    else
      raise 'ResultIsNilError'
    end
  end
end

Somewhere this nil has to be dealt with. Either here, or somewhere later in the Action chain, or it could even bubble up to the UI.

I was thinking about using an extension to the promised macro. What do you think about this?

class Action
  include LightService::Action
  promised! :something
  executed do |ctx|
    result = SomeOperation.call
    unless result.nil?
      ctx.something = result
    else
      raise 'ResultIsNilError'
    end
  end
end

The promised! macro would guarantee that the value added to the Context hash would not be nil. The promised macro however would allow nils. Not sure how I feel about it, but I wanted to run it by you.

from light-service.

@padi: what do you suggest we do with this issue?

from light-service.

@adomokos: How about adding an option that's similar to Rails ActiveRecord, allow_nil?

class Action
  include LightService::Action
  promises :something, allow_nil: false
  executed do |ctx|
    result = SomeOperation.call
    unless result.nil?
      ctx.something = result
    else
      raise 'ResultIsNilError'
    end
  end
end

... but I guess that would entail setting the default value to be true i.e. not allowing nil from the start. An advantage would be that the intent is obvious based on the syntax, as opposed to adding a exclamation mark (!) to promises method.

from light-service.

I've been OK without having this feature, but if somebody feels the need of having a contract for promises and expects key -> values not be nil, I'd support the '!' option.

class SomeAction
  extends LightService::Action
  promises! :foo
  expects! :bar

  executed do |ctx|
   ...
  end
end

These macros would check if the value represented by the key is nil, and if is, it would throw an exception, protecting the actions from dancing around nils.

@padi: would that work?

from light-service.

@adomokos I'm hesitant about using ! since it doesn't immediately communicate the intent. I use/define bang methods sparingly because they have a specific purpose. They usually denote that the bang method is the more dangerous method (contrary to popular belief that it's always the version that modifies the receiver of the method). To quote Matz:

The bang (!) does not mean "destructive" nor lack of it mean non
destructive either. The bang sign means "the bang version is more
dangerous than its non bang counterpart; handle with care". Since
Ruby has a lot of "destructive" methods, if bang signs follow your
opinion, every Ruby program would be full of bangs, thus ugly.

(Source: https://www.ruby-forum.com/topic/176830#773946)

Example: ActiveRecord::Model#save! skips validations vs ActiveRecord::Model#save. I'm of the opinion that allowing nil is more dangerous than not allowing it, so promises! is counter-intuitive.

from light-service.

I hear that you don't like the bang, and I am not a big fan of :allow_nil => false.

I think we have to allow nils by default. I recently had to code a mapping logic like this:

class SomeAction
  extend LightService::Action
  expects :source_document
  promises :some_result

  executed do |ctx|
    ctx.some_result = ctx.source_document.fetch(:a_key)
  end
end

Here, source_document => {:a_key => nil} will produce ctx.some_result = nil. Which is perfectly fine, as that value will be serialized into a JSON or written to a DB nullable field.

I could see expects having a rule of not being nil, but when you try to interact with them, they are going to fail anyway.

You are not a big fan of '!', I don't really like :allow_nil=>false as it's too verbose, then what other options do we have here?

from light-service.

If it comes down to it, I'll probably just write an extension of light-service. Not eager to make it part of the 1.0 release.

from light-service.

Sounds good. I'll close this issue now and feel free to reopen it or open a new one when you think you're ready. 👍

from light-service.