Add support for overriding default CUD opperations #69
Conversation
| end | ||
|
|
||
| def call(input) | ||
| command.call(root => input) | ||
| command.call(command.name.relation => input) | ||
| end |
There was a problem hiding this comment.
Hmm I think this change won't cause issues but I'm wondering what made you change this?
There was a problem hiding this comment.
The reason is because the previous value was:
Dry::Core::Inflector.singularize(command.name.relation).to_sym
Which meant, if the command was for relation :users the command was being called with:
command.call(:user => { ... })
But the tuple_path in the input elevator called when executing a number of the commands was looking for :users. Changing it to command.name.relation instead meant that the commands were always called like:
command.call(users: { ... })
Which provided the right key for the tuple_path in the InputEvaluator to be traversed.
I imagine it would have been better to update how the tuple_path was constructed but I have 0 idea how to do that.
There was a problem hiding this comment.
Right, makes sense not to do something that's not really needed :D
This class is a temporary "solution", the only reason why it exists is because command graphs in rom core, as you noticed, need the relation name as the key. If we can tweak it so this is no longer needed, then this whole proxy can be removed.
There was a problem hiding this comment.
Reading that code comment made me feel a little better about changing this class. I think we're not far off making this obsolete. Could you point me in the direction of some cool bits of the command graph code to read?
There was a problem hiding this comment.
It is probably not that difficult to tweak the code in core so that we can ask graph builder to build the command where the first one is marked as a root without a tuple path. Basically here we ALWAYS append a key to the path (and path is empty by default) that's how we end up with evaluator that always expects a tuple path. So we'd have to change two things:
There was a problem hiding this comment.
Perhaps if the path is empty, InputEvaluator should just return the input? Then we could just not add the key to the path?
I don't understand how the path is built up though.
There was a problem hiding this comment.
@jasper-lyons yeah it crossed my mind to just allow nil but it's a bit nasty, because we'd be adding special meaning to nil and IF it's nil when it shouldn't be we'd be wondering why and debugging would be harder. So I'd say it's better to use input evaluator when there's actually something to evaluate, otherwise just pass the input as it is.
I don't understand how the path is built up though.
It's built by traversing through the command graph ast and building it up as we dive deeper, see the spec where option is a sample ast, we have :users then :tasks so we end up with :'users.tasks' tuple path while traversing through these options.
There was a problem hiding this comment.
So I'd say it's better to use input evaluator when there's actually something to evaluate, otherwise just pass the input as it is.
Completely agree. Lets do that.
tweak these two lines so that if
pathisfalsethen we use an input evaluator that will simply return the input (literally -> input { input })
Why not when path is empty?
There was a problem hiding this comment.
Why not when path is empty?
Because it would break public API
.gitignore
Outdated
| @@ -1,3 +1,5 @@ | |||
| Gemfile.lock | |||
| log/*.log | |||
| doc | |||
| .byebug_history | |||
| tags | |||
There was a problem hiding this comment.
Vim or Emacs?
BTW, you can configure global gitignore for all projects ever to ignore such files
There was a problem hiding this comment.
Vim.
I should definitely do that. Thanks.
| end | ||
| end | ||
|
|
||
| class CreateAccount < ROM::Commands::Create[:memory] |
There was a problem hiding this comment.
If I could workout how to do this without creating a stand alone constant I could bring this code more inline with the existing tests. @solnic?
There was a problem hiding this comment.
you can use configuration to define a command using rom's DSL:
configuration.commands(:users) do
define(:create)
end
I use a cludgy `Object.send(:remove_const, :CustomCommand)` here to allow the naming of the command object which make checking the type of the command returned easy. This could be implemented with some duck typing perhaps?
Moved it into my global ignore instead.
I use a cludgy `Object.send(:remove_const, :CustomCommand)` here to allow the naming of the command object which make checking the type of the command returned easy. This could be implemented with some duck typing perhaps?
Moved it into my global ignore instead.
This follows on from #68.
I would like some feedback on how to improve the testcase and bring it more inline with the existing tests if possible!