search

fernandobatels / rsfbclient

Rust Firebird Client
MIT License
76 stars 11 forks source link

Diesel #86

Closed fernandobatels closed 3 years ago

fernandobatels commented 4 years ago

Todo:

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 600331109

Changes Missing Coverage Covered Lines Changed/Added Lines %
rsfbclient-diesel/src/fb/query_builder.rs 47 49 95.92%
rsfbclient-diesel/src/fb/value.rs 15 24 62.5%
rsfbclient-native/src/connection.rs 20 29 68.97%
src/connection/simple.rs 6 15 40.0%
rsfbclient-diesel/src/fb/transaction.rs 18 29 62.07%
src/connection/mod.rs 61 74 82.43%
rsfbclient-diesel/src/fb/connection.rs 45 59 76.27%
rsfbclient-rust/src/client.rs 22 36 61.11%
rsfbclient-diesel/src/fb/types.rs 74 107 69.16%
<!-- Total: 643 757 84.94% -->
Files with Coverage Reduction New Missed Lines %
src/connection/simple.rs 4 30.39%
src/transaction/simple.rs 5 25.53%
<!-- Total: 9 -->
Totals Coverage Status
Change from base Build 592132101: -0.7%
Covered Lines: 2480
Relevant Lines: 3157
💛 - Coveralls
jairinhohw commented 3 years ago

@fernandobatels please take a look at my changes. I've created a default transaction on the Connection type, so that there is no need to create and store a Transaction in the TransactionManager. Also changed the behavior of the Queryable and Executable methods in case the new begin_transaction method on the Connection was called and there is an active transaction.

Edit:. Now it only fails in the coverage, but this is probably some problem of the tarpaulin with threaded code

fernandobatels commented 3 years ago

@fernandobatels please take a look at my changes. I've created a default transaction on the Connection type, so that there is no need to create and store a Transaction in the TransactionManager. Also changed the behavior of the Queryable and Executable methods in case the new begin_transaction method on the Connection was called and there is an active transaction.

I liked the changes. Previously I thought about creating a begin_transaction that would return a new transaction, but the idea of a default transaction is very nice.

My only concern is with the use_transaction method. I'm afraid that this may cause some confusion with the with_transaction method. Is it necessary to be public?

Edit:. Now it only fails in the coverage, but this is probably some problem of the tarpaulin with threaded code

I'm running the CI again, to see if this persists

jairinhohw commented 3 years ago

My only concern is with the use_transaction method. I'm afraid that this may cause some confusion with the with_transaction method. Is it necessary to be public?

It can be private, as my initial idea was to use it directly in the rsfbclient-diesel crate, but then i created the begin_transaction, commit and rollback functions and forgot to remove the pub on the use_transaction.

I will try to find a better test that fails in the older version without using threaded code to see if tarpaulin stops complaining

jairinhohw commented 3 years ago

I've replaced the multithreaded test with a simpler version that causes the same segfault on the version with unsafe code, hopefully tarpaulin will not complain anymore

fernandobatels commented 3 years ago

I rebased the branch, to improve the history commits

fernandobatels commented 3 years ago

Note: When this PR is approved, I will open a PR for the diesel repo with my changes.

dancingbug commented 3 years ago

Could someone open a new PR to merge https://github.com/fernandobatels/rsfbclient/tree/diesel_split with master, and add me as reviewer? (If I create it, I can't add myself as reviewer)

It's a subset of this branch corresponding to all the non-diesel changes