Ntj/nuo db data reader #37
Conversation
dd further explanatory comments.
| @@ -76,7 +76,7 @@ private void InitResultSet(int handle, EncodedDataStream dataStream, bool readCo | |||
| this.values = new Value[numberColumns]; | |||
| this.closed = false; | |||
| this.currentRow = 0; | |||
| this.afterLast = false; | |||
| // this.afterLast = false; | |||
There was a problem hiding this comment.
This line was in a block that previously set ALL the member fields.
I agree that leaving a commented line is poor style.
I'll replace the commented line with a comment that explains why afterLast is not being set now.
The alternative was to set afterLast = false here, as originally done, and then set its value later.
But, I thought was was potentially confusing.
| return false; | ||
|
|
||
| //int maxRows = statement == null ? 0 : statement.MaxRows; | ||
| int maxRows = 0; | ||
| int maxRows = 0; // this local maxRows HIDES this.maxRows |
There was a problem hiding this comment.
If this variable is hiding a class field, why not rename it?
There was a problem hiding this comment.
I regard this as a hack,
It was in the original code - I didn't make this change originally.
I left this highly dubious code for now, because I wanted just to implement the needed feature in this PR - and not get embroiled in cleaning this code up and ensuring the correct behaviour remains.
In a later PR, I will fix this code by removing the hiding, and cleaning up the code that relies on the hidden var.
To answer you question, there is a condition later in this method that tests maxRows, so hiding maxRows with a local zero (0) value effectively disables that test.
Which is the intention of this code.
I think the long term solution is to just make sure this class works correctly in the absence of a reliable maxRows value.
Implement NuoDbDataReader.HasRows
Newer EFCore code relies on DataReader.hasRows.
However, our original implementation in NuoDbDataReader threw a NotImplemented exception.
This implementation correctly handles the case where the response from the TE contains zero (0) rows.