Skip to content

Conversation

@SuperBuker
Copy link

Issue:
Some register_session parameters are not being propagated through get_smb_tree, which limits their usage.

Changelog:

  • Adding auth_protocol and require_signing to the get_smb_tree connection kwargs.

@SuperBuker
Copy link
Author

Hi,

I've noticed that the project code usually makes use of $a or $b to check if the provided value $a is not None.
Sadly, this code effectively returns $b if $a is also one of the following: str(), bool(), int(), float(), list(), dict() and set(), which can lead to undesired side effects.

I've written the code using $a or $b, but I strongly suggest to avoid this pattern and replace it with $a if $a is not None else $b even if it's ugly.

Thanks in advance.

All the best,
Jorge

@adiroiban
Copy link
Contributor

adiroiban commented Jan 26, 2026

Thanks, Jorge, for the changes. They look good.

This is just a 3rd party review. I am not the maintainer of this project.
Just trying to do some review so that Jordan will have less work.


I think that these are important changes and they are worth a mention in the release notes.
I think that this PR should also update

https://github.com/jborean93/smbprotocol/blob/master/CHANGELOG.md


Regarding:

auth_protocol = auth_protocol or client_config.auth_protocol

I think that for the new code, we can start using better practices.
Yes, it will not be consistent with the existing code, but at least the new code will be of a better quality/safety.
I think that code safety should come first, and consistenty second.


Would it be possible to add some tests to make sure that these arguments work as expected and that we will not introduce regression with future refactoring ?

Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants