Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ldap-checker: fix for Python 3.12 compatibility #270

Merged
merged 1 commit into from
Apr 21, 2024
Merged

ldap-checker: fix for Python 3.12 compatibility #270

merged 1 commit into from
Apr 21, 2024

Conversation

exploide
Copy link
Contributor

The ldap-checker module is currently not compatible with Python 3.12+ because ssl.wrap_socket() was deprecated and has been removed. Fixed this by using SSLContext.wrap_socket() instead.

While doing so, my code linter was complaining so I added a cleanup and code quality commit first. The commits are separate to make reviewing a cleanup commit and a compatibility fix commit easier.

@NeffIsBack
Copy link
Contributor

Thanks for the fix! I'll check it out soon. I would let the elif statements though, there is no reason to check the error multiple times if one matches.

@NeffIsBack NeffIsBack added the bug-fix This Pull Request fixes a bug label Apr 21, 2024
@exploide
Copy link
Contributor Author

I would let the elif statements though, there is no reason to check the error multiple times if one matches.

The thing is, there is always a return right before the next if, so it will stop anyway if it evaluates to True once. That's why the linter was complaining.

However, if any stylistic change is not at yours discretion, please let me know. I can change that of course. :)

@NeffIsBack
Copy link
Contributor

I would let the elif statements though, there is no reason to check the error multiple times if one matches.

The thing is, there is always a return right before the next if, so it will stop anyway if it evaluates to True once. That's why the linter was complaining.

However, if any stylistic change is not at yours discretion, please let me know. I can change that of course. :)

Makes sense, i would leave it anyway though, because it is clearer, that we can enter only one if statement.
Btw which linter do you use?

@exploide
Copy link
Contributor Author

Okay, just stripped the cleanup commit.

I'm currently using pylint. I know you are using ruff, and I use it on a project at work, too. But haven't switched to it entirely, yet.

Copy link
Contributor

@NeffIsBack NeffIsBack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could reproduce on py3.12.3:
image

Patch LGTM for 3.12 and also 3.11:
image

@NeffIsBack
Copy link
Contributor

@exploide thanks for the report and fix!

@NeffIsBack NeffIsBack merged commit 1f8a0ef into Pennyw0rth:main Apr 21, 2024
5 checks passed
@exploide exploide deleted the ldap-checker-py3.12 branch April 22, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This Pull Request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants