Session execute_read/write implementation #251
Conversation
JuliaKovtun
force-pushed
the
impl-execute-read-write
branch
from
6a47c16 to
db04fb7
Compare
There was a problem hiding this comment.
Our spec write transaction function retries deadlocks corresponds to java test writeTransactionFunctionShouldRetryDeadlocks. That test is marked as deprecated. I don't see any tests testing the same functionality with executeWrite in java so we should not either. Let's exclude this test.
Furthermore, I think it would make sense to disable the deprecation message in test environment for readability.
| check do | ||
| super( | ||
| ->(tx) { Struct::Wrapper.new(reverse_check { block.call(tx) }) }, | ||
| to_java_config(Neo4j::Driver::TransactionConfig, **config) | ||
| ).object | ||
| end |
There was a problem hiding this comment.
I would extract this to a separate private method. So the above logic can be simplified.
| %i[read write].each do |mode| | ||
| ["#{mode}_transaction", "execute_#{mode}"].each do |method_name| | ||
| define_method(method_name) do |**config, &block| | ||
| if method_name.include? 'transaction' |
There was a problem hiding this comment.
This is kind of backwards and sort of duplication.
I think in this case it would be better to have 2 separate method definitions. One with deprecation and another without.
| super( | ||
| ->(tx) { Struct::Wrapper.new(reverse_check { block.call(tx) }) }, | ||
| to_java_config(Neo4j::Driver::TransactionConfig, **config) | ||
| ).object |
There was a problem hiding this comment.
let's increase our max line length to 160. That's what we have on all our projects.
|
|
||
| check do | ||
| super( | ||
| ->(tx) { Struct::Wrapper.new(reverse_check { block.call(tx) }) }, |
There was a problem hiding this comment.
Please check if Struct::Wrapper is still required.
There was a problem hiding this comment.
okay, will check
| # implementation of read_transaction, write_transaction, execute_read, execute_write | ||
| %i[read write].each do |mode| | ||
| define_method("execute_#{mode}") do |**config, &block| | ||
| execute_transaction(__method__, **config, &block) | ||
| end | ||
|
|
||
| define_method("#{mode}_transaction") do |**config, &block| | ||
| Neo4j::Driver::Internal::Deprecator.log_warning("#{mode}_transaction", "execute_#{mode}".to_sym, '6.0') | ||
| execute_transaction(__method__, **config, &block) |
There was a problem hiding this comment.
ouch! That got complex. I didn't realize that when making the recommendation.
Seeing that I would prefer to duplicate the method implementation or go back to the conditional deprecation you had until we remove the method.
| end | ||
|
|
||
| define_method("#{mode}_transaction") do |**config, &block| | ||
| Neo4j::Driver::Internal::Deprecator.log_warning("#{mode}_transaction", "execute_#{mode}".to_sym, '6.0') |
There was a problem hiding this comment.
You can use relative namespace
| Neo4j::Driver::Internal::Deprecator.log_warning("#{mode}_transaction", "execute_#{mode}".to_sym, '6.0') | |
| Internal::Deprecator.log_warning("#{mode}_transaction", "execute_#{mode}".to_sym, '6.0') |
There was a problem hiding this comment.
I tried Internal::Deprecator.log_warning but it raised a NameError, so left here Driver::Internal::Deprecator.log_warning
JuliaKovtun
changed the title
#247