Use trailblazer-option over declarative-option and upgrade declarative-builder #492
Conversation
| # This is identical to `Representable::Option`. | ||
| module Cell | ||
| # Extend `Trailblazer::Option` to support static values as callables too. | ||
| class Option < ::Trailblazer::Option |
There was a problem hiding this comment.
@apotonick This is identical to Representable::Option.
Should we move Cell::Option and Cell::Options in trb-option instead ?
There was a problem hiding this comment.
Hm, do we have use on a generic level?
There was a problem hiding this comment.
Hm, kind of.
- Handling static values might be needed somewhere else.
- But
Cell::Optionsisn't generic. We can keep it here.
There was a problem hiding this comment.
The question is, do we have the static requirement anywhere else (e.g. Reform)? If yes, we can extract it. On the other hand, we can always do that later. 🥱
There was a problem hiding this comment.
Yep, we've the static requirement in reform, disposable and representer. Currently, it is done in representer itself and it's being used in other 2.
There was a problem hiding this comment.
On the other hand, supporting Callable objects between these 2 types of gems (representable, reform, cells etc and trb-activity, DSL etc) is little different i.e.
- Extending
Uber::Callablefor identifying callable object. - trb-activity and friends doesn't need static type support.
So other way is we should not mix these 2 types in trb-option but keep separate. If it's ok, then we can keep Cell::Option here.
There was a problem hiding this comment.
- Reg.
Uber::Callable, that one we should dump ASAP. Isn't it better if we always treat an objectcallable, anyway? - Hm, but it also doesn't hurt to have it there instead of maintaining 4 different
Options, right?
There was a problem hiding this comment.
Hm yeah. The problem with treating every object callable is we can't distinguish between them and static options (some static options also includes modules, classes etc), apart from call method.
There was a problem hiding this comment.
Ahhh, ok right, so that was why I initially introduced Uber::Callable, damn! Hm... how could we do that in the future?
Ok, in that case, we won't move that behavior to TRB, for now. 🍰
There was a problem hiding this comment.
Correct (i.e. distinction between static and callable objects isn't needed in TRB). So we can merge this PR.
Uber::Callable is a nice alternative to mark callables for cells and representable friends for now.
In future versions, maybe we can have restrictions on static options (or ways to avoid them) ?
lib/cell/option.rb
Outdated
| # Extend `Trailblazer::Option` to support static values as callables too. | ||
| class Option < ::Trailblazer::Option | ||
| def self.callable?(value) | ||
| [Proc, Symbol, Uber::Callable].any?{ |kind| value.is_a?(kind) } |
There was a problem hiding this comment.
I'm not sure about this change. We quite frequently pass symbols as cell options, is this going to to try and evaluate them as a method name?
There was a problem hiding this comment.
@samstickland This behaviour is same as the previous Declatative::Option. The difference with Trailblazer::Option in the API only, and it's compatible with Ruby-3.
There was a problem hiding this comment.
@yogeshjain999 We do have static values covered in the cells suite, I guess? 😅
There was a problem hiding this comment.
Oh really ? I thought those are covered in declarative-option directly.
declarative-builderversion (referring to the reported issue)declarative-optionwithtrailblazer-optionasdeclarative-builderuses same.