inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

Add is_closed() #68

Closed Geobert closed 3 years ago

Geobert commented 3 years ago

Fixes #67 Add the is_closed() method to check the underlying connection state. On behalf of Isode Ltd.

inejge commented 3 years ago

I'm not against this in principle, but I'll have to think about it. Meanwhile:

Geobert commented 3 years ago

After your comment in the issue, maybe that is not needed :)

I'll test the WhoAmI and if not satisfied, I'll address your comment :)

I don't understand why it should be &mut self though, the call has no chance to modify self, so why?

inejge commented 3 years ago

After your comment in the issue, maybe that is not needed :)

Maybe not, but as I noted in the other comment, "state" is not a single thing and knowing that a connection is open has its uses, so don't close the PR or the sibling issue yet.

I don't understand why it should be &mut self though

For uniformity, although technically it doesn't have to be.

Geobert commented 3 years ago

For uniformity, although technically it doesn't have to be.

Then I disagree on putting mut, the method signature should carry the information if there's a risk of modifying the object or not (like void MyClass::my_method() const; in C++)

inejge commented 3 years ago

I stand by the uniformity argument; if it makes it more palatable for you, regard &mut self as an exclusivity marker. (This is needed for making SearchStream exclusively derived from the Ldap handle.)

Geobert commented 3 years ago

In the end of the day, it's your project so I'll conform to your wish :) I added is_closed() to sync.rs as well :)

Geobert commented 3 years ago

Hi,

Is that in a merge-able state? Or do you want me to change something else?

inejge commented 3 years ago

It's mergeable and I'm working on it.

Geobert commented 3 years ago

Thank you very much :D

inejge commented 3 years ago

Merged with some documentation touchups, thanks.