zim32 / mysql.dart

MySQL client for Dart written in Dart
BSD 3-Clause "New" or "Revised" License
64 stars 17 forks source link

Add simple getter to check if IResultSet has data #28

Closed MHShetty closed 2 years ago

MHShetty commented 2 years ago

Merging this pull request shall fix issue #8.

This pull request just adds a bool getter to check if a result set has data, which would make it easier to achieve code readability in certain use-cases (for e.g. where only checking whether records that matches a certain criteria is required).

Please test the changes and let me know if any changes are required.

Thanks for your valuable time @zim32!

zim32 commented 2 years ago

Why do wee need all this overrides? To clarify what we have now:

ResultSet - for simple query() IterableResultSet - for iterable results, where data must be accessed from Stream: result.rowsStream.listen(...) PreparedStmtResultSet - this is almost like ResultSet but for prepared statements IterablePreparedStmtResultSet - this is almost like IterableResultSet but for prepared statements EmptyResultSet - represents empty result, f.e. for UPDATE or DELETE statements.

So, for iterable result set classes, I thinks it is impossible to check for presense of data, because they are designed to just emit whatever they read from network, without storing rows somewhere. So I think they must throw excpetion if calling hasData on them with some reasonable message.

EmptyResultSet is OK. Just return false. ResultSet and PreparedStmtResultSet must override basic implementation. Just return _resultSetPacket.rows.isNotEmpty. I guess something like that.

MHShetty commented 2 years ago

Why do wee need all this overrides?

The code throws error while compiling, expecting the derived classes to implement the getter otherwise. (possible because the IResultSet class is abstract and how Dart works)

To clarify what we have now:

ResultSet - for simple query() IterableResultSet - for iterable results, where data must be accessed from Stream: result.rowsStream.listen(...) PreparedStmtResultSet - this is almost like ResultSet but for prepared statements IterablePreparedStmtResultSet - this is almost like IterableResultSet but for prepared statements EmptyResultSet - represents empty result, f.e. for UPDATE or DELETE statements.

So, for iterable result set classes, I thinks it is impossible to check for presense of data, because they are designed to just emit whatever they read from network, without storing rows somewhere. So I think they must throw excpetion if calling hasData on them with some reasonable message.

Yes this makes sense, should have tried to understand the classes better before making those changes for all the derived classes in general.

ResultSet and PreparedStmtResultSet must override basic implementation. Just return _resultSetPacket.rows.isNotEmpty. I guess something like that.

Sure, I'll make the required changes to the branch soon, do a bit of self-review and revert back asap.

Thanks a lot for briefing me through the required changes @zim32!

zim32 commented 2 years ago

Thanks for contribution

MHShetty commented 2 years ago

Hi @zim32,

I think this pull request closed when I was trying to re-create the has_data_check branch and I'm unable to do re-open this issue despite (since GitHub is considering them uniquely different). I'll just make a new PR once the existing set of issues get resolved,

Thanks for contribution

It's my pleasure )

Thanks for considering this PR @zim32!